cookie icon indicating copy to clipboard operation
cookie copied to clipboard

Add ESM build script

Open PaulKiddle opened this issue 2 years ago • 5 comments

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

PaulKiddle avatar Oct 17 '23 16:10 PaulKiddle

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.

dougwilson avatar Oct 17 '23 17:10 dougwilson

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 avatar Oct 18 '23 08:10 PaulKiddle

@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 avatar Oct 23 '23 11:10 PaulKiddle

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

PaulKiddle avatar Nov 07 '23 16:11 PaulKiddle

@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.

CallMeLaNN avatar Dec 04 '23 05:12 CallMeLaNN