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

Add ESM build script #154

Closed
wants to merge 2 commits into from
Closed

Add ESM build script #154

wants to merge 2 commits into from

Conversation

PaulKiddle
Copy link

Adds a script to create the ESM version of the code, fixing #152

The script is automatically run on prepare (i.e. when packaging for release or installing directly from git) on npm versions ≥ 4
The generated file is not committed to the repository

@dougwilson
Copy link
Contributor

Awesome, thank you! I know there has been past discussions, so not sure if you have seen all of it, but a couple things for this PR i noticed missing, if you can think of how we can solve:

  1. I added needs tests bc we want to make sure that however we are generating this actually reaults in an importable module in our ci insteqd of waiting for users to open an issue after a publish. Also may need tweaks down the line and so having a safety net in tests is always desirable.
  2. Many of the security vendors that stare at the Express.js source code explicitly want us to have the files in the tarball on npm to be the exact files checked into the github repository at the commit hash that is in the npm metadata, so all these express used module we try to uphold that as best as we can, to keep their score high (as typically my understanding from them is untracked code in npm tarballs is considered suspicious by nature).

In addition, one of the pervious issues was that why does this build step need to exist at all? Node.js provides a way to have both cjs ans esm without any build steps needed. The last time it was due to Vite but the user decided it was a Vite bug and not to fix it here. Does that apply still, or can we simply follow the Node.js docs like https://nodejs.org/api/packages.html#approach-1-use-an-es-module-wrapper ?

Myself and I'm sure anyone else consuming esm is really thankful.

@PaulKiddle
Copy link
Author

PaulKiddle commented Oct 18, 2023

Thanks for the feedback, those changes make sense.

As for why this is needed, my use case was Rollup which by default doesn't support common js (even via the wrapper method suggested in the Node docs). There is a plugin to enable it which I currently use, but if I can reduce my dependencies and config complexities, I would rather do that.

Plus I believe that given ESM modules have been the standard for a while we will start to see more build tools and other environments that do not support CJS, so being prepared for that possibility would be beneficial.

@PaulKiddle
Copy link
Author

@dougwilson It should be simple to run all the tests twice (once for each export type) by wrapping them in a function or loop (any preference?)
Would probably also need to distinguish if a test is running in ESM or CJS mode by adding something to the describe and/or it messages. I think just adding litterally (esm) or (cjs) to the describes should suffice - does that sound fine?

@PaulKiddle
Copy link
Author

FYI I am waiting for confirmation that this approach sounds good before I go ahead and do the work to implement this

@CallMeLaNN
Copy link

@dougwilson I think your confirmation is needed. I only have this dep that that don't have ESM support. @PaulKiddle thanks, hope you still helping this.

@blakeembrey
Copy link
Member

Hi @PaulKiddle, thanks for the PR! I'm trying to tidy up some older PRs in this repo and I'm going to close this one as we won't be adding dual package publishing in the immediate future. As far as I understand, most bundlers support CommonJS. I think this will make sense to revisit if/when we go ESM only.

@blakeembrey blakeembrey closed this Oct 2, 2024
@balazsorban44 balazsorban44 mentioned this pull request Nov 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants