fix cjs with nodenext interop issue

[Uzlopak]

Resolves #389

I tried to keep a centralized types-folder in dist, but unfortunately but after 2h of trial and error, this was the most straight forward way to fix the issue. Now we have typings next to the index.js for esm and cjs and it works.

[Uzlopak]

@EvanHahn
@kibertoad
@climba03003

PTAL

[EvanHahn]

I'll look soon, but probably won't have time in the next few days.

[climba03003]

Have you double check the emitted file using npm run build-helmet.
Since the tsconfig.json is using "module": "NodeNext", it will emit ESM types in both folder.
The CJS file is transpile by rollup from ESM to CJS instead of TypeScript.

[climba03003]

It is actually why I remove rollup in all my package.
The types is not reflect what the actual files when coupling with rollup.

[Uzlopak]

Yes.

I used your example code and created a new test-project.
You can clone it here:
https://github.com/Uzlopak/helmet-test

Then I did npm run build-helmet in my fork and branch of helmet and then did npm link and in my test-project I did npm link helmet

Then I just did "npm run build" in my test-project with the all combinations of
package.json - type: "module", type: "commonjs"
tsconfig.json - compilerOptions.moduleResolution: "Node", compilerOptions.moduleResolution: "NodeNext"

[Uzlopak]

@climba03003
I would not rip out rollup because I assume @EvanHahn would not be OK with it.

[climba03003]

Yes.

Have you look inside the type file? It may works depends on setting, or bug in TypeScript. But the emitted types is not the correct one.

[Uzlopak]

You mean because of the .js suffix of the imports?

[climba03003]

I means the types of CJS is incorrect. It is the same version as ESM.
The problem still persist in this case.

[Uzlopak]

Even with typescript original tsc it will result in the same typings.

Modify in helmet the tsconfig-commonjs.json include to index.ts

run npx tsc -p tsconfig-common.json

It doesnt drop the .js suffix.

But the last line you claim is missing is with direct use of tsc there

[Uzlopak]

The last line of exports is removed on purpose by @EvanHahn

See

helmet/bin/helpers.js

Line 53 in ca35562

const startLine = lines.findIndex((line) =>

[climba03003]

npx tsc -p tsconfig-common.json is emiting ESNext types.
It inherit the setting of tsconfig.json

[Uzlopak]

Doesnt actually matter. It works in helmet-test.

The only thing, I think is relevant is the question if I should remove the code, which suppresses the export of the Interfaces for CJS

[climba03003]

After deep dive into TypeScript document. I see how the resolution of types works.

Old Code

When the project is CommonJS, "exports.require" is used for JavaScript and since there is no *.d.ts files. It will fallback to types field which means the package.json#module is ESM.
Here is the cause why the types are the same but treats as ESM.

New Code

When the project is CommonJS, "exports.require" is used for JavaScript and TypeScript found the *.d.ts files. It use the resolved types and reference to the dist/cjs/package.json#module.
So, it is consider as CommonJS.

Remark

The package.json#exports.types is placed in last place and it is actually ignored by TypeScript. But, when you follow the guide and place at top. It break the above theory to use the *.d.ts files beside the *.js file.
So, either keep it or remove it do not affect the code.

Regrading the types is not export = in CommonJS. I do not found any reference on why it is happened. But when I look through my projects, it is the same. I will not border it much.

[kibertoad]

Perhaps, tsd tests or some simple TS file with self-import that would be compiled in CI, could be added here, that would have been failing previously?

[Uzlopak]

@kibertoad
@climba03003

https://github.com/Uzlopak/helmet/actions/runs/3601124357/jobs/6066567174

I guess some combinations make no sense and thats why it is failing?

[kibertoad]

yeah, looks like it

[Uzlopak]

Here for comparison against the main branch

https://github.com/Uzlopak/helmet/actions/runs/3601170615

[EvanHahn]

I plan to review later this month. Sorry for the delay.

[Uzlopak]

@EvanHahn

I ignore in the workflow the failing cases. So if @climba03003 or somebody tries to improve the interoperability, it should be possible to remove that specific case from the in the workfllow. But on the other hand it will be mergable at the current state and it is having now a test, so a regression should be impossible.

https://github.com/Uzlopak/helmet/actions/runs/3693726445

[EvanHahn]

Could someone show me a library that does something similar? I want to make sure I don't break any existing workflows by integrating this change.

[Uzlopak]

I dont know. I wrote this without any template.

[aqibgatoo]

Is there some quick fix till this gets merged?

[Uzlopak]

@EvanHahn

Wazzup? Any ETA?

[EvanHahn]

No ETA, sorry. I've been busy. I'll try to get to this soon.

[Uzlopak]

@EvanHahn

Hi, we got another issue opened because of this issue. fastify/fastify-helmet#216

Can you spare some time to review, please?

[EvanHahn]

Yes, sorry for my slowness. I'll plan to review by Friday.

[EvanHahn]

I modified this branch and made #405. Please try it out by running:

npm install 'https://evanhahn.com/tape/helmet-6.0.1.tgz'

Let me know if this works for you. If it does, I'll release a new version.

[EvanHahn]

I'm closing this in favor of #405. Please tell me if that works for you (or doesn't). I'll be merging that PR soon.

[EvanHahn]

I modified this and deployed it in [email protected]. Thanks for your help!