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

Improve support for various TypeScript setups, including "nodenext" #405

Merged
merged 11 commits into from
Apr 8, 2023

Conversation

EvanHahn
Copy link
Member

Please try this out by running npm install 'https://evanhahn.com/tape/helmet-6.0.1.tgz'.

Closes #389 and #391.

When I merge this, I'll make @Uzlopak the author.

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 15, 2023

Thats strange. I get

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

> [email protected] build
> tsc -p .

node_modules/@fastify/helmet/types/index.d.ts:2:18 - error TS2614: Module '"helmet"' has no exported member 'contentSecurityPolicy'. Did you mean to use 'import contentSecurityPolicy from "helmet"' instead?

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

@EvanHahn
Copy link
Member Author

@Uzlopak Could you show me a sample project that reproduces your issue?

It looks like you're trying to import @fastify/helmet...is that using this build?

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 15, 2023

i use https://github.com/Uzlopak/nodenext-mvce with the branch helmet-issue

@EvanHahn
Copy link
Member Author

EvanHahn commented Mar 15, 2023 via email

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 15, 2023 via email

@yi-fan-song
Copy link

I've got a question for this, can we remove the "type": "module" in package.json, it resolves the issue for me, and I'm not seeing why that needs to be set unless this package itself uses module code.

It makes typescript think this package is ES module only, and refuses to resolve the cjs types even when I compile with target commonjs. That only happens when using moduleResolution: nodenext/node16/node18

@EvanHahn
Copy link
Member Author

@Uzlopak Look for helmet in your node_modules directory. Is there a dist/types/ folder there?

@yi-fan-song Are you suggesting we remove "type": "module" from Helmet? Or from your own code?

@yi-fan-song
Copy link

yi-fan-song commented Mar 16, 2023 via email

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 16, 2023

I dont think that removing the type: module from helmet is useful, as we are moving to esm. But we should be enable cjs for cjs modules. And this is the reason why we have this PR.

That is actually also the reason why it fails in @fastify/helmet, as @fastify/helmet is cjs and not esm.

I will have a look at your changes @EvanHahn and will come back to you asap

@Uzlopak
Copy link
Contributor

Uzlopak commented Mar 18, 2023

@EvanHahn
I am sorry but I got a new Appartment and I have to renovate it. I am totally blocked by this... :-/

@EvanHahn
Copy link
Member Author

I just made a few changes. Please try it out with npm install 'https://evanhahn.com/tape/helmet-6.0.1.tgz' and let me know what you think.

@EvanHahn
Copy link
Member Author

I'm going to merge this soon. Please let me know if this fails to work for you.

@EvanHahn EvanHahn self-assigned this Mar 28, 2023
@EvanHahn
Copy link
Member Author

EvanHahn commented Apr 2, 2023

I intend to make some more updates here but they're not quite ready.

@EvanHahn EvanHahn merged commit 5bdac6e into main Apr 8, 2023
@EvanHahn EvanHahn deleted the module-systems branch April 8, 2023 16:15
@EvanHahn
Copy link
Member Author

EvanHahn commented Apr 8, 2023

This has been published in [email protected]. Thanks to everyone for their help!

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

Successfully merging this pull request may close these issues.

TS1479, helmet typings issue in combination with fastify
3 participants