-
Notifications
You must be signed in to change notification settings - Fork 371
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
typescript issues #474
Comments
How are you importing Helmet? Could you show me a code snippet that can reproduce this problem? |
import { City, Country, State } from 'country-state-city';
import helmet from 'helmet';
import polka from 'polka';
import bearerToken from 'polka-bearer-token';
const app = polka();
const port = process.env.PORT || 3000;
app
.use((req, res, next) => {
res.setHeader('Content-Type', 'application/json');
res.setHeader('Access-Control-Allow-Origin', '*');
res.setHeader('Access-Control-Allow-Methods', 'GET, OPTIONS');
res.setHeader('Access-Control-Allow-Headers', 'Content-Type, Authorization');
next();
})
.use(
helmet({
crossOriginResourcePolicy: false,
})
)
.use(bearerToken())
.use((req, res, next) => {
const token = (req as typeof req & { token: string }).token.replace('Bearer ', '');
if (token !== process.env.TOKEN) {
res.statusCode = 401;
res.end(JSON.stringify({ message: 'Unauthorized', status: 401 }));
}
next();
})
.get('/countries', (req, res) => {
res.end(JSON.stringify(Country.getAllCountries()));
})
.get('/countries/:id', (req, res) => {
res.end(JSON.stringify(Country.getCountryByCode(req.params.id)));
})
.get('/states', (req, res) => {
res.end(JSON.stringify(State.getStatesOfCountry(req.query.country as string)));
})
.get('/states/:id', (req, res) => {
res.end(JSON.stringify(State.getStateByCode(req.params.id)));
})
.get('/cities', (req, res) => {
res.end(JSON.stringify(City.getCitiesOfState(req.query.country as string, req.query.state as string)));
})
.get('*', (req, res) => {
res.statusCode = 404;
res.end(JSON.stringify({ message: 'Not Found', status: 404 }));
});
app.listen(port, () => {
console.log(`Started on ${port}`);
}); |
I couldn't reproduce this in a reduced sample app. Here's what I have:
{
"private": true,
"scripts": {
"typecheck": "tsc --noEmit"
},
"dependencies": {
"helmet": "^7.1.0"
},
"devDependencies": {
"typescript": "^5.6.2"
}
}
{
"include": ["app.ts"],
"compilerOptions": {
"target": "ES2020",
"module": "Node16",
"moduleResolution": "Node16",
"types": [],
"resolveJsonModule": true,
"allowJs": true,
"sourceMap": true,
"importHelpers": true,
"allowSyntheticDefaultImports": true,
"esModuleInterop": true,
"forceConsistentCasingInFileNames": true,
"strict": true,
"noImplicitAny": false,
"skipLibCheck": true
}
}
import helmet from "helmet";
console.log(
helmet({
crossOriginResourcePolicy: false,
}),
); What versions are you using? Are any of your dependencies outdated? What about your version of Node.js? |
@EvanHahn I made a reproducible example: https://codesandbox.io/p/devbox/r8t9gx The sandbox is using node v20.9.
A potential fix could be to export helmet additionally as a named export. // index.ts of helmet package
- const helmet: Helmet = Object.assign(
+ export const helmet: Helmet = Object.assign(
function helmet(options: Readonly<HelmetOptions> = {}) { More context on how another package has solved a similar issue with module resolution "node16". postcss/postcss#1815 |
@marc-wittwer Thanks. I'm able to reproduce the issue you're seeing and I'll try to fix it soon. |
Same issue here |
I don't know why this is happening, but it seems to be a ts-node issue and not a Helmet issue. (Might be wrong, though!) On @marc-wittwer's repo, when I run I don't know why that is. Does anybody have any ideas? Does the problem persist if you switch to another tool like |
@EvanHahn - Best I can tell, your suggestion that this is a I believe I've narrowed the issue down to "exports": {
"import": "./index.mjs",
"require": "./index.cjs"
}, and I get the same error as mentioned above when attempting to call // don't do this
"exports": {
"default": "./index.mjs",
"import": "./index.mjs",
"require": "./index.cjs"
}, suddenly the error is resolved. So, it seems like I got the following idea from Node's documentation about different approaches to dual CJS and MJS support: "exports": {
".": {
"import": "./index.mjs",
"require": "./index.cjs"
},
"./module": "./index.mjs"
}, This keeps existing conditional exports for the vast majority of users that import helmet from "helmet/module"; What do you think? |
Make it possible to import the ESM module from 'helmet/module' to support older tools that don't well support conditional exports (e.g. ts-node). Duplicate ESM tests to ensure importing from 'helmet/module' works as expected. Fixes helmetjs#474
@mdmower-csnw Thanks for this investigation and for your PR. I haven't read either in detail, but will look when I get a chance. |
I haven't investigated this deeply, but I think I'd rather fix the bug in As a workaround, could you import from the file directly? Something like this: import helmet from "helmet/index.mjs"; @mdmower-csnw Your thorough investigation makes me think we could file a bug on |
@EvanHahn - there's a problem with that approach:
This is the background for my comment above:
|
I neglected to respond to your first idea: I don't think this will work because you've defined
and attempting to run the source with
|
I'm sympathetic to an unmaintained dependency causing issues—this is happening to my in a personal project right now!—but I don't think Helmet should add workarounds for Instead, you could manually patch Helmet to fix the bug. You could install diff --git a/node_modules/helmet/package.json b/node_modules/helmet/package.json
index 4088f52..be02b75 100644
--- a/node_modules/helmet/package.json
+++ b/node_modules/helmet/package.json
@@ -39,10 +39,7 @@
"engines": {
"node": ">=18.0.0"
},
- "exports": {
- "import": "./index.mjs",
- "require": "./index.cjs"
- },
- "main": "./index.cjs",
- "types": "./index.d.cts"
+ "exports": "./index.mjs",
+ "main": "./index.mjs",
+ "types": "./index.d.mts"
} I've forked the CodeSandbox to show a working solution. I'm going to close this issue because I think this is a reasonable workaround for |
sorry to say this has come up again
tsconfig
result
The text was updated successfully, but these errors were encountered: