backend: Fix import of sanitize-html

Michcio commented

2022-10-10 18:54:55 +00:00

Contributor

I'm not sure how it managed to work so far, but the function is the default
export, using the namespace like a function should not have worked,
maybe something under the hood was correcting it back

I'm not sure how it managed to work so far, but the function is the default export, using the namespace like a function should not have worked, maybe something under the hood was correcting it back

Michcio added 1 commit 2022-10-10 18:54:56 +00:00

backend: Fix import of sanitize-html

ci/woodpecker/pr/build Pipeline was successful

Details

ci/woodpecker/pr/lint-backend Pipeline was successful

Details

ci/woodpecker/pr/lint-foundkey-js Pipeline was successful

Details

ci/woodpecker/pr/lint-client Pipeline failed

Details

ci/woodpecker/pr/test Pipeline failed

Details

eb369f4e88

I'm not sure how it managed to work so far, but the function is the default
export, using the namespace like a function should not have worked,
maybe something under the hood was correcting it back

Johann150 commented

2022-10-10 21:10:25 +00:00

Owner

The packages NPM page says:

If esModuleInterop=true is not set in your tsconfig.json file, you have to import it with:
import * as sanitizeHtml from 'sanitize-html';

The said option is not set in packages/backend/tsconfig.json.

The [packages NPM page](https://www.npmjs.com/package/sanitize-html) says: > If `esModuleInterop=true` is not set in your `tsconfig.json` file, you have to import it with: > ```js > import * as sanitizeHtml from 'sanitize-html'; > ``` The said option is not set in `packages/backend/tsconfig.json`.

Michcio commented

2022-10-10 22:52:58 +00:00

Author

Contributor

The package's NPM page also states that TypeScript support is close to none, and the issue that caused it was over a year ago. From the thread it seems the maintainers just accepted whatever helped somebody on the thread and slapped it on the top of the readme. I tried just now building a minimal repro case of it. What I do know is:

sanitize-html does not have "type": "module" in package.json, while we do. This is known to cause NodeJS to handle the imports and exports differently, for the package specifically, but this is independent for each package.
Our tsconfig.json implores tsc to resolve modules through node, and to generate code in the model of esnext ("type": "module") style modules.

When I build a tiny project that just runs sanitizeHtml("aaa") with those two facts included in configs, the code only works if I remove * as. Because when we're in "type": "module", and tsc knows that we are, NodeJS handles the difference in module model behind the scenes completely. When there is a discrepancy between what tsc and what NodeJS think the module model to be, only then is there a need for a workaround like the * as.

So with that in mind I believe esModuleInterop is not actually necessary, here or elsewhere, in otherwise well configured TS projects with "type": "module".

Why, I've ran a main branch test instance just now, enabled email wherever I could in the admin panel, and what I get in the logs when submitting an abuse report is

TypeError: sanitizeHtml is not a function
    at Immediate.<anonymous> (file:///Users/michcioperz/Code/FoundKey-0x7f/packages/backend/built/server/api/endpoints/users/report-abuse.js:90:55)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

because apparently this is and has been an untested code path? Idk. When I remove as * the error disappears (and turns into an SMTP error, because of course I didn't configure SMTP, but it passes the sanitization part).

The package's NPM page also states that TypeScript support is close to none, and the issue that caused it was over a year ago. From the thread it seems the maintainers just accepted whatever helped somebody on the thread and slapped it on the top of the readme. I tried just now building a minimal repro case of it. What I do know is: - `sanitize-html` does not have `"type": "module"` in `package.json`, while we do. This is known to cause NodeJS to handle the imports and exports differently, for the package specifically, but this is independent for each package. - Our `tsconfig.json` implores tsc to resolve modules through node, and to generate code in the model of esnext (`"type": "module"`) style modules. When I build a tiny project that just runs `sanitizeHtml("aaa")` with those two facts included in configs, the code **only** works if I **remove** `* as`. Because when we're in `"type": "module"`, and tsc knows that we are, NodeJS handles the difference in module model behind the scenes completely. When there is a discrepancy between what tsc and what NodeJS think the module model to be, only then is there a need for a workaround like the `* as`. So with that in mind I believe esModuleInterop is not actually necessary, here or elsewhere, in otherwise well configured TS projects with `"type": "module"`. --- Why, I've ran a `main` branch test instance just now, enabled email wherever I could in the admin panel, and what I get in the logs when submitting an abuse report is ``` TypeError: sanitizeHtml is not a function at Immediate.<anonymous> (file:///Users/michcioperz/Code/FoundKey-0x7f/packages/backend/built/server/api/endpoints/users/report-abuse.js:90:55) at processTicksAndRejections (node:internal/process/task_queues:96:5) ``` because apparently this is and has been an untested code path? Idk. When I remove `as *` the error disappears (and turns into an SMTP error, because of course I didn't configure SMTP, but it passes the sanitization part).

Johann150 commented

2022-10-11 08:39:27 +00:00

Owner

Ok yeah I also checked and there is a type error about this when building the backend as is, which is fixed by this patch.

Johann150 manually merged commit 60600729df into main

2022-10-11 08:43:15 +00:00

backend: Fix import of sanitize-html #193