Skip to content
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

TS1479, helmet typings issue in combination with fastify #389

Closed
2 tasks done
Uzlopak opened this issue Nov 25, 2022 · 19 comments · Fixed by #405
Closed
2 tasks done

TS1479, helmet typings issue in combination with fastify #389

Uzlopak opened this issue Nov 25, 2022 · 19 comments · Fixed by #405

Comments

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 25, 2022

In combination with fastify, I get this error.
fastify/fastify-helmet#210

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.10.2

Plugin version

10.1.0

Node.js version

16.17.0

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

22.04

Description

It seems that we get typescript errors, because the typings from helmet are ecmascript and not commonjs

uzlopak@Battlestation:~/workspace/nodenext-mvce$ npm run build

> nodenext-test@1.0.0 build
> tsc -p .

node_modules/@fastify/helmet/types/index.d.ts:2:62 - error TS1479: The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("helmet")' call instead.

2 import helmet, { contentSecurityPolicy, HelmetOptions } from 'helmet';
                                                               ~~~~~~~~


Found 1 error in node_modules/@fastify/helmet/types/index.d.ts:2

Steps to Reproduce

clone https://github.com/Uzlopak/nodenext-mvce/tree/helmet-issue
npm run build

Expected Behavior

Should not throw

@Uzlopak Uzlopak changed the title TS1479, helmet typings issue TS1479, helmet typings issue in combination with fastify Nov 25, 2022
@EvanHahn
Copy link
Member

This looks like an issue with fastify-helmet and not Helmet itself, so I'm going to close. Let me know if that's wrong and I'll reopen!

@EvanHahn EvanHahn closed this as not planned Won't fix, can't repro, duplicate, stale Nov 25, 2022
@climba03003
Copy link

climba03003 commented Nov 25, 2022

@EvanHahn
The problem is helmet only provide ESM types but not CJS types.
When using with "type": "module" in package.json and "module": "NodeNext", "moduleResolution": "Node16" in TypeScript. The mismatch of types will throw an error, sine TypeScript think that it is a ESM module only.

@EvanHahn
Copy link
Member

EvanHahn commented Nov 25, 2022 via email

@climba03003
Copy link

Do you experience this problem with Helmet directly

Yes.

// tsconfig.json
{
  "compilerOptions": {
    "moduleResolution": "NodeNext"
  }
}

// index.ts
import helmet from 'helmet'

helmet

// package.json
{
  "dependencies": {
    "helmet": "^6.0.0"
  },
  "devDependencies": {
    "@types/node": "^18.11.9",
    "typescript": "^4.9.3"
  }
}

@EvanHahn
Copy link
Member

Thanks. I'm able to reproduce the problem, but I think this working as expected. I don't think you should be able to import helmet from 'helmet' in CommonJS mode. If you add "type": "module" to your package.json, it should work.

Does that help?

@climba03003
Copy link

climba03003 commented Nov 25, 2022

Does that help?

No, it is the problem of mis-match of types.
You can use import helmet from 'helmet', but TypeScript provide more combination then you though.
Forcing people to use certain configuration is not optimal for a library. And the solution is as simple as provide the correct types for both CJS and ESM.

TypeScript itself it blaming library from serving the incorrect types harder and harder when it evolving.

@climba03003
Copy link

climba03003 commented Nov 25, 2022

If you add "type": "module", the problem solve only to suppress the TypeScript error.
But, when you check the output of file. Yes, it is using CJS as intended and the package.json is telling Node.JS to run as ESM.

Thus, the problem is not solved at all. But bring to a runtime error.

@EvanHahn
Copy link
Member

Are there libraries that do a better job here? I have had trouble making things work seamlessly with CommonJS + ESM + TypeScript.

@climba03003
Copy link

climba03003 commented Nov 25, 2022

Are there libraries that do a better job here?

Not the best example, but it is how I do in TypeScript. https://github.com/climba03003/fastify-bree
When you provide the *.d.ts file next to *.js, TypeScript actually prefer that types file first. (Maybe, it is the extra package.json trigger this behavior.)

So, the easiest way should be emit proper types under the dist/cjs.

@kibertoad
Copy link

Maybe this should be reopened, since issue still exists, and requires a solution?

@EvanHahn
Copy link
Member

Maybe this should be reopened, since issue still exists, and requires a solution?

Done!

I won't have time to investigate this in 2022. If anyone would like to submit a pull request, I'm happy to take a look!

@xx745
Copy link

xx745 commented Mar 20, 2023

Still a problem, for anyone who comes across this issue. TS5 + Nodejs 19

@EvanHahn
Copy link
Member

I believe I have fixed this in #405. Please try that to see if it fixes your issue.

@xx745
Copy link

xx745 commented Apr 1, 2023

@EvanHahn sorry, but I'm still getting the error:

module "/home/.../Documents/.../gt-be/node_modules/helmet/dist/esm/esm" The current file is a CommonJS module whose imports will produce 'require' calls; however, the referenced file is an ECMAScript module and cannot be imported with 'require'. Consider writing a dynamic 'import("helmet")' call instead. To convert this file to an ECMAScript module, change its file extension to '.mts', or add the field"type": "module"to '/home/.../Documents/.../gt-be/package.json'.ts(1479)
I'm using
`
"packageManager": "[email protected]",
"dependencies": {
"cors": "^2.8.5",
"dotenv": "^16.0.3",
"express": "^4.18.2",
"helmet": "https://evanhahn.com/tape/helmet-6.0.1.tgz",
"mongodb": "^5.1.0"
},
"engines": {
"node": ">=19.8.1"
},
"devDependencies": {
"@types/cors": "^2.8.13",
"@types/express": "^4.17.17",
"@types/jest": "^29.5.0",
"@types/node": "^18.15.3",
"@types/supertest": "^2.0.12",
"@typescript-eslint/eslint-plugin": "^5.55.0",
"@typescript-eslint/parser": "^5.55.0",
"eslint": "^8.36.0",
"jest": "^29.5.0",
"nodemon": "^2.0.20",
"rimraf": "^4.4.0",
"supertest": "^6.3.3",
"ts-jest": "^29.0.5",
"ts-node": "^10.9.1",
"typescript": "^5.0.2"
}
}

@EvanHahn
Copy link
Member

EvanHahn commented Apr 1, 2023

Thanks for testing this. What does your tsconfig.json look like?

@xx745
Copy link

xx745 commented Apr 1, 2023

@EvanHahn

{
"compilerOptions": {
/* Visit https://aka.ms/tsconfig to read more about this file /
"target": "ES2022", /
Set the JavaScript language version for emitted JavaScript and include compatible library declarations. /
"module": "CommonJS", /
Specify what module code is generated. /
"rootDir": ".", /
Specify the root folder within your source files. /
"moduleResolution": "node16", /
Specify how TypeScript looks up a file from a given module specifier. /
"resolveJsonModule": true, /
Enable importing .json files. /
"declaration": true, /
Generate .d.ts files from TypeScript and JavaScript files in your project. /
"sourceMap": true, /
Create source map files for emitted JavaScript files. /
"outDir": "build", /
Specify an output folder for all emitted files. /
"esModuleInterop": true, /
Emit additional JavaScript to ease support for importing CommonJS modules. This enables 'allowSyntheticDefaultImports' for type compatibility. /
"forceConsistentCasingInFileNames": true, /
Ensure that casing is correct in imports. /
"strict": true, /
Enable all strict type-checking options. /
"noImplicitAny": true, /
Enable error reporting for expressions and declarations with an implied 'any' type. /
"strictNullChecks": true, /
When type checking, take into account 'null' and 'undefined'. /
"noImplicitThis": true, /
Enable error reporting when 'this' is given the type 'any'. /
"noUnusedLocals": true, /
Enable error reporting when local variables aren't read. /
"noUnusedParameters": true, /
Raise an error when a function parameter isn't read. /
"noImplicitReturns": true, /
Enable error reporting for codepaths that do not explicitly return in a function. /
"noFallthroughCasesInSwitch": true, /
Enable error reporting for fallthrough cases in switch statements. /
"skipLibCheck": true /
Skip type checking all .d.ts files. /
},
"include": ["src"],
"exclude": ["src/**/
.test.ts", "build/**", "node_modules"]
}

@EvanHahn
Copy link
Member

EvanHahn commented Apr 2, 2023

Thanks. I am working on a fix for this.

@EvanHahn
Copy link
Member

EvanHahn commented Apr 8, 2023

This should be fixed in [email protected].

@xx745
Copy link

xx745 commented Apr 9, 2023

@EvanHahn - yes, it's working now, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants