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

TypeScript types are clashing with nodenext TypeScript module resolution #359

Closed
philkunz opened this issue Mar 24, 2022 · 15 comments
Closed

Comments

@philkunz
Copy link

TypeScript throws Relative import paths need explicit file extensions in EcmaScript imports when '--moduleResolution' is 'node12' or 'nodenext'. Consider adding an extension to the import path. when using helmet in esm context and module resolution mode node12 or nodenext.

The package.json specifies

"exports": {
    ".": {
      "import": "./dist/esm/index.js",
      "require": "./dist/cjs/index.js",
      "types": "./dist/types/index.d.ts"
    }
  }

This requires corresponding types for esm, that also use file extensions for imports

@EvanHahn
Copy link
Member

Sorry about this.

The files in ./dist/esm and ./dist/cjs/ don't contain any imports, but the files in ./dist/types/ do. I suspect this is happening because of lines like this:

import contentSecurityPolicy, { ContentSecurityPolicyOptions } from "./middlewares/content-security-policy"

I'm not sure of the best way to fix this, but I'm sure there's a good way. Does anyone have any ideas?

@philkunz
Copy link
Author

simply use the correct nodenext module resolution with TypeSciprt. That should solve things.

@philkunz
Copy link
Author

I can take a look next week.

@EvanHahn
Copy link
Member

Great, thanks!

I won't have time to fix this soon, so thanks for taking a look.

@anonrig
Copy link

anonrig commented Jun 2, 2022

Is there any priority for this particular task?

@EvanHahn
Copy link
Member

EvanHahn commented Jun 2, 2022

I haven't had time to look into it, sadly. Would you like to try making a fix?

I'm not sure I know the best way to fix things because of the way Helmet is built, but I'm sure there's a good way.

@EvanHahn
Copy link
Member

Does anyone have a fix for this? If not, does anyone have a small repo that can reproduce this problem?

@EvanHahn EvanHahn added this to the v6.0.0 milestone Jun 25, 2022
@Pauline-sh
Copy link
Contributor

Pauline-sh commented Jul 2, 2022

I used a patch like that to use helmet with nodenext modules. Just added extension to every import in declaration file

--- "node_modules\\.pnpm\\helmet@5.1.0\\node_modules\\helmet\\dist\\types\\index.d.ts"	2022-06-28 00:53:22.576028800 +0300
+++ helmet.index.d.ts	2022-07-02 22:54:15.916937700 +0300
@@ -1,20 +1,20 @@
 /// <reference types="node" />
 import { IncomingMessage, ServerResponse } from "http"
-import contentSecurityPolicy, { ContentSecurityPolicyOptions } from "./middlewares/content-security-policy"
-import crossOriginEmbedderPolicy from "./middlewares/cross-origin-embedder-policy"
-import crossOriginOpenerPolicy, { CrossOriginOpenerPolicyOptions } from "./middlewares/cross-origin-opener-policy"
-import crossOriginResourcePolicy, { CrossOriginResourcePolicyOptions } from "./middlewares/cross-origin-resource-policy"
-import expectCt, { ExpectCtOptions } from "./middlewares/expect-ct"
-import originAgentCluster from "./middlewares/origin-agent-cluster"
-import referrerPolicy, { ReferrerPolicyOptions } from "./middlewares/referrer-policy"
-import strictTransportSecurity, { StrictTransportSecurityOptions } from "./middlewares/strict-transport-security"
-import xContentTypeOptions from "./middlewares/x-content-type-options"
-import xDnsPrefetchControl, { XDnsPrefetchControlOptions } from "./middlewares/x-dns-prefetch-control"
-import xDownloadOptions from "./middlewares/x-download-options"
-import xFrameOptions, { XFrameOptionsOptions } from "./middlewares/x-frame-options"
-import xPermittedCrossDomainPolicies, { XPermittedCrossDomainPoliciesOptions } from "./middlewares/x-permitted-cross-domain-policies"
-import xPoweredBy from "./middlewares/x-powered-by"
-import xXssProtection from "./middlewares/x-xss-protection"
+import contentSecurityPolicy, { ContentSecurityPolicyOptions } from "./middlewares/content-security-policy/index.js"
+import crossOriginEmbedderPolicy from "./middlewares/cross-origin-embedder-policy/index.js"
+import crossOriginOpenerPolicy, { CrossOriginOpenerPolicyOptions } from "./middlewares/cross-origin-opener-policy/index.js"
+import crossOriginResourcePolicy, { CrossOriginResourcePolicyOptions } from "./middlewares/cross-origin-resource-policy/index.js"
+import expectCt, { ExpectCtOptions } from "./middlewares/expect-ct/index.js"
+import originAgentCluster from "./middlewares/origin-agent-cluster/index.js"
+import referrerPolicy, { ReferrerPolicyOptions } from "./middlewares/referrer-policy/index.js"
+import strictTransportSecurity, { StrictTransportSecurityOptions } from "./middlewares/strict-transport-security/index.js"
+import xContentTypeOptions from "./middlewares/x-content-type-options/index.js"
+import xDnsPrefetchControl, { XDnsPrefetchControlOptions } from "./middlewares/x-dns-prefetch-control/index.js"
+import xDownloadOptions from "./middlewares/x-download-options/index.js"
+import xFrameOptions, { XFrameOptionsOptions } from "./middlewares/x-frame-options/index.js"
+import xPermittedCrossDomainPolicies, { XPermittedCrossDomainPoliciesOptions } from "./middlewares/x-permitted-cross-domain-policies/index.js"
+import xPoweredBy from "./middlewares/x-powered-by/index.js"
+import xXssProtection from "./middlewares/x-xss-protection/index.js"
 export interface HelmetOptions {
 	contentSecurityPolicy?: ContentSecurityPolicyOptions | boolean
 	crossOriginEmbedderPolicy?: boolean

Can make a PR with adding these extensions to the source .ts file if it's ok

@EvanHahn
Copy link
Member

EvanHahn commented Jul 3, 2022

@Pauline-sh Thanks for this. I'll take a look.

@Pauline-sh
Copy link
Contributor

And the repo for reproduction https://github.com/Pauline-sh/helmet-ts-bug-repro

@EvanHahn
Copy link
Member

EvanHahn commented Jul 7, 2022

Thanks for the repro! To set expectations: I'm busy this week but should have time to look after that.

EvanHahn pushed a commit that referenced this issue Jul 22, 2022
This fixes a TypeScript module resolution bug.

See [#373][373] and [issue 359][359].

[373]: #373
[359]: #359
@EvanHahn
Copy link
Member

I'm working on this now. Sorry for the long delay.

@EvanHahn
Copy link
Member

I haven't been able to figure out the issue, but maybe someone else can help.

My goal is to add file extensions to dist/types/index.d.ts. No other files need to change. Basically, here's what needs to be done:

+import contentSecurityPolicy, { ContentSecurityPolicyOptions } from "./middlewares/content-security-policy"
-import contentSecurityPolicy, { ContentSecurityPolicyOptions } from "./middlewares/content-security-policy/index.js"

Applying #373 addresses that, and it works!

Unfortunately, Jest doesn't work when you do that. For reasons unknown to me, it doesn't resolve modules the same way, and gives errors with the .js extension. (It does work when there's a built dist/ directory, but not if that directory is missing.)

I see two reasonable solutions, neither of which I've been able to figure out:

  1. Get Jest to work with these extensions. This is my preference.
  2. Add .js extensions to the imports in the built files (but keep them out of the source files).

There are some other solutions that might work, but I think those are my favorite choices.

If anyone can help figure this out or has thoughts, I'd appreciate it. Otherwise I'll keep working on it later.

@Pauline-sh
Copy link
Contributor

Yeah, looks like I ran the tests with built dist and haven't noticed this issue :(

I tried to adjust jest-config according to this doc https://kulshekhar.github.io/ts-jest/docs/guides/esm-support/ and I think it worked. Will make a PR with this solution

EvanHahn pushed a commit that referenced this issue Jul 23, 2022
@EvanHahn
Copy link
Member

This should be fixed in Helmet v5.1.1. Please update and let me know if that fixes things for you.

Sorry I took so long on this! And big thanks to @Pauline-sh for doing all the work.

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

No branches or pull requests

4 participants