Improve CJS/ESM interoperability support #345
Conversation
|
Thanks for this. It's a nontrivial change so it might be a few days before I review this, apologies! |
There was a problem hiding this comment.
This looks reasonable at a first glance...thanks for doing this! I'll take a more thorough look soon.
I'll test it myself but I want to double-check that the following work:
- Vanilla JS,
let helmet = require('helmet')should work - Vanilla JS with
"type": "module"inpackage.json,import helmet from 'helmet'andimport * as helmet from 'helmet'should work - TypeScript, compiling to CommonJS without
esModuleInterop - TypeScript, compiling to CommonJS with
esModuleInterop - TypeScript, compiling to ECMAScript modules
MitchellCash
force-pushed
the
cjs-esm-interop
branch
from
b35d263
to
18906e7
Compare
|
I'll take another look at this in a few days. (Things are a bit busy for me and I want to make sure I don't skim over this!) |
bin/helpers.js
Outdated
| export async function renameFile(oldPath, newPath) { | ||
| await fs.rename(oldPath, newPath) | ||
| .catch((error) => { | ||
| if (error) console.error(error); | ||
| }); | ||
| } |
There was a problem hiding this comment.
It seems strange that we'd drop errors here—I think I'd like it to crash. Could we delete this function and just use fs.rename?
bin/build-helmet.js
Outdated
| ); | ||
| await moveDir(path.join(esmDistDir, "middlewares"), path.join(typesDistDir, "middlewares")) |
There was a problem hiding this comment.
Why can't we just fs.rename this? Why do we need this fancy helper?
There was a problem hiding this comment.
fs.rename doesn't handle moving files to directories that don't exist, nor have any sort of recursive renaming AFAIK. So this function was created to recursively traverse a directory tree and create required directories before moving a file.
However, in commit c8e887c I've removed this function in an attempt to simplify things.
Instead of outputting the *.d.ts type files into dist/esm/* and then using the moveDir helper to get them over to dist/types/*. Now when outputting ESM I output the types straight to dist/types/* and then only need to worry about moving dist/index.js to dist/esm/index.js.
We get to the same place as before, but with less code for us to maintain and a little more readable as we only are moving a single file instead of traversing a directory tree moving many files. Happy for feedback!
|
@EvanHahn happy to help! Also, apologies I've been busy the past few days, I will go over your feedback today 😃 |
|
Looks like you've been making some changes—let me know when it's time for me to take another look. |
|
@EvanHahn ready for the next review now, I believe all the previous feedback should now be resolved. By the way, if you would like me to squash these commits at any point please let me know. |
|
Great, I'll take a look soon.
No need to squash commits.
|
There was a problem hiding this comment.
Huge thanks for all of your help here! I'll merge and release soon.
Feel free to add yourself to the contributors list, or let me know your preferred name and optionally a URL, and I'll put you on there myself.
|
This has been released in |
Resolves #344 (hopefully).
The real main difference here is we have some additional CJS exports in
index.ts, but these exports aren't compatible with ESM so have to be removed in thebuild-helmet.jsprocess.I made a local package with
npm run build-helmet && npm packand went through the few test cases in https://github.com/helmetjs/helmet/blob/main/MAINTAINER_README.md and all seemed to pass. It also resolved my issue detailed in the above issue thread without changing mytsconfig.jsonor the way I importhelmet. Which on top of fixing the issue also means there is no longer anything breaking changed for me fromv4tov5.I welcome any feedback and if it helps any of the other people in the above thread.