[RRFC] Validate ESM before npm publish

Motivation ("The Why")

Many npm packages contain invalid ESM, which makes Node.js choke.

The situation is quite bad, as described in nodejs/node#46074.

Example

For example, the npm package @aws-amplify/ui-react contains invalid ESM, see aws-amplify/amplify-ui#3155.

How

Current Behaviour

Npm packages can be published while containing invalid ESM.

Desired Behaviour

Validate npm packages before they are published: npm should reject any npm publish that publishes invalid ESM.

References

For reference, https://publint.dev is a project that validates whether an npm package is published correctly.

npm just wraps up a tarball and publishes it - it doesn't validate CJS packages, why would it validate ESM ones? What about packages that support multiple node versions - how would npm validate "not the current node version"?

@ljharb

@ljharb The bottom line is that a lot of npm packages are being published with invalid ESM that make Node.js choke. The question here is: what can we do about it? As discussed in nodejs/node#46074, we don't want to make Node.js permissive if we don't strictly need to. So that leaves us with the only option of making npm strict about the code it hosts. Or maybe you're seeing another solution I'm not seeing?

The situation is untenable and we really need a solution. (I'm the author of vite-plugin-ssr and the, by far, number 1 issue users are reporting is Node.js chocking on npm packages having invalid ESM.)

What about packages that support multiple node versions - how would npm validate "not the current node version"?

The validation doesn't have to cover everything. Only validating basics things like the following would make a big impact already:

a widespread problem is npm packages having invalid ESM imports, e.g. import './foo' instead of import './foo.js', which makes Node.js choke. Another example is packages that ship ESM but don't have type: "module" in their package.json. Node.js should be able to execute npm packages that ship invalid code.

npm can't and shouldn't be strict about what it hosts, and i very much agree node can't and shouldn't be permissive - so the only solution is education. Very few packages even need to be ESM (or should be!) and that's part of education too.

I understand npm should be agnostic. But I'm thinking the situation is so bad that it warrants an exception.

As much as I'd like education to be the solution, I'm afraid it doesn't cut it. I already have documentation about invalid ESM on https://vite-plugin-ssr.com but it doesn't make much of a difference. I'm afraid nothing will happen if we close this ticket. On the other hand, if npm validates ESM, then that would completely resolve the situation. That'd be a godsend for a lot of users. Also a godsend for library maintainers (e.g. I've already reached the Apollo GraphQL team 3 times because they repeatedly shipped invalid ESM code).

To quote a user hitting a Node.js issue because an npm package has invalid ESM:

Now there is some cryptic node bug that showed up out of nowhere
JS libraries are a special kind of punishment

The situation is painful.

I'm thinking that the (only?) way forward here is to make npm validate ESM.

Based on the discussion in that node thread, it seems like the reason it's painful it's vite's decision to "draw a line in the sand". It doesn't seem problematic with other bundlers - am i missing something?

Vite transpiles only what is strictly necessary (that's why it's so much faster). This means that with Vite, Node.js directly executes code (whereas Next.js transpiles the code before Node.js executes it). That's why the situation is worse in the Vite ecosystem.

It also affects folks equally badly who directly use Node.js (without a bundler like Next.js/webpack).

right, but the pain you're describing seems like it is the very necessity that needs more transpilation.

-1 on having npm check this. This should be tooling built by the ecosystem, not something Node.js nor npm maintain. Also, having npm do this would not actually solve the problem - there are other package managers 😅

One of Vite's foundational innovation is "lazy-transpiling": only transpile what is strictly necessary. Transpiling all npm packages only because some contain invalid ESM is a massive performance degradation. Vite values performance a lot and this isn't going to change.

I think everyone can agree that we should foster valid ESM, and that it would be a good thing if npm hosts ESM that is valid. (It doens't have to be 100% valid: a few basic checks can get us a long way.)

It's also nice for folks who don't use bundlers (e.g. Node.js servers without frontend) to be able to directly use npm packages without the need for a transpiler.

Making npm validate ESM is a very effective way of achieving this.

I understand the general principle of keeping npm agnostic but I strongly believe we should do an exception as I think the pros outweigth the cons, by far.

The most important here: it's a net win for everyone, starting from the users who suffer from this. Also including library authors who will be relieved to know that they shipped valid ESM. (I can imagine that the Apollo GraphQL would love this.)

That's fine - but a package using the "module" field, and not the "exports" field, is one that it is necessary to transpile. Similarly, if a package is published with syntax that isn't supported by the target environments, the only choices are to transpile it, or, not use that package - "use it as-is" isn't a viable choice.

@brillout

@brillout a good place to start might be having package authors opting into some kind of validation (like publint that you mentioned), e.g:

$ npm pkg set "scripts.prepublishOnly=npx publint"

This might be a simple patch to submit to a few projects/teams (like in your example with the Apollo GraphQL team) to try and see if you can get some meaningful improvement to the ecosystem prior to having it builtin to package managers.

The way I see it is this: it's either pain for (Vite / Node.js server) users, or npm validates ESM.

I'd love an alternative as much as you do, but I don't see any.

The unfortunate truth is that users are suffering, and I wonder what we can we do about it.

@ruyadorno

@ruyadorno Sounds nice, although it's virtually impossible to educate the entire ecosystem to do this. That's why I believe in npm validating ESM, and so by default.

I don't get the sentiment that it's outside the scope of the Node Package Manger to check that packages it publishes are compatible with Node. Potentially you could say it should be the registry responsible for this rather than the CLI, but the idea that we're okay with a majority of packages being broken and it's the consumers job' to fix those packages by running them through bundlers instead of running them on Node is a very anti-Node position for the Node Package Manager to be taking.

I think a lot of the confusion here may be that the issue title refers to ESM, but this really isn't just an ESM issue. You can write invalid CJS packages as well. And we see that happen constantly. E.g. "type": "module" and CJS code with .js extension - the CJS code won't run on Node. Node packaging has gotten more complicated since the introduction of exports, .cjs/.mjs, etc. and we see packages published to npm that won't run on Node on a non-stop basis.

I strongly agree that the goal of keeping npm agnostic has given the ability for developers to ship invalid packages to the registry and poisoning the transition from CJS to ESM.

I’m sad to see that an OSS maintainer had to open an issue to the most used js runtime, and later to npm (owned by github, owned by ms) to solve a validation problem caused by the initial goal of “agnostic” path npm is taken. This is clearly a validation problem.

As a Node core collaborator, and performance enthusiast, from where I’m sitting (after reading all of the responses), I strongly believe that we should do better. Since Node does not have the capacity to validate npm packages before publishing, I believe it should definitely be done in the package manager side.

While I share the sentiment of keeping the npm registry as simple as possible, I don't see an alternative to adding validation to the npm registry. Let me elaborate.

There are four approaches:

We make Node.js permissive (i.e. making Node.js be able to execute npm packages that have invalid ESM).

This is being categorically rejected, see Make Node.js more permissive when libraries ship invalid ESM code nodejs/node#46074.
We encourage library authors to publish valid ESM (e.g. with scripts.prepublishOnly or by encouraging them to write ESM tests).

As much as I'd love this work, I don't think it's feasible to educate enough users to make a real difference here. To quote benmccann (emphasis mine):

I work on SvelteKit and we get tons of bugs saying a given package is broken with SvelteKit when it's really not related to SvelteKit at all, but just Node.js. I've spent an inordinate amount of time educating users that their packages are not following the Node.js spec, written an FAQ on our site, fixed tons of widely-used packages in the ecosystem, but I'll just never reach the long tail of users.

With the massive amount of invliad npm packages being published every day, we just don't have the manpower to fix the situation with education alone.
We add validation to the npm CLI.

I agree this is problematic as it would create fracture between npm/yarn/pnpm.
We add validation to the npm registry (e.g. the registry rejects with a 400, while having an opt-out option).
- I believe this should be opt-out: e.g. npm publish --skip-validation / npm publish --force would make the npm registry accept everything like it does today.
- While adding complete validation would be nice (e.g. by testing packages against Node.js), I think we can already get very far with fairly simple checks. For example, detecting import { someImport } from './some-file' (it should be ./some-file.js) only for the files referenced by package.json#exports would already be an enormous added value. We can start simple and see how far we can go.

That's why I've come to the conclusion that the only viable option here is 4: we add validation to the npm registry.

And, actually, the more I think about it, the more I like the idea. I find it very compelling to have a registry that is clean. That'd be a much welcome and tremendous added value of the npm registry.

npm currently gets sent a tarball and some JSON data. I'm not sure if they even unpack it at all, except perhaps for the relatively new "explore"/"core" feature on package webpages. I suspect it's not feasible, in a practically performant/efficient way, to implement this on the registry side.

I don't think option 3 has to cause any fracturing. It could be implemented as a shared module

@ljharb

@ljharb it's an excellent time to start; validating income tarballs is very important and not so heavy. It will be possible to reject viruses and maybe abuse material. The npm registry can't avoid DCMA and GDPR, so not to get serious troubles should be a way to validate received packages.

Of course it can, it’s a DMCA safe harbor only if it doesn't actively scan the contents of what users upload - otherwise it would have potential red-flag knowledge of violations. I’m not sure how GDPR plays in. Either way, a lawyer would have to weigh in there.

@bnb

the last days left me dumbfounded by the amount of pushback, derailing and hardly even acknowledging the issue presented by members of tc39, npm inc and github.

@bnb out of my post you chose to quote 2 statements and apply twists, conveniently not getting to my question regarding alternative options.

I disagree with you that what i said is based on a fallacy of composition but arguing about that isn't going to add any value here so please let's agree to disagree here.
This has nothing to do with the dynamic typed nature of js, so i don't understand why that is being brought into the discussion.

Sharing the validation logic can be done, which solves the concerns about being agnostic. I am thankful you care about that, but at the same time you should acknowledge that node.js bundling npm is indeed preferential treatment and has lead to your position that the npm registry and client are by far the most used in the js ecosystem and as a result applying validation at that level is the most effective.

Completely agree that it should be the registry itself performing the ultimate steps, but clientside validation isn't a new concept. Look at html form validation.
Which coincidentally can be shared between server and client more easily than ever before thanks to properly authored esm packages that can be executed vanilla by browsers, node.js and various js based edge runtimes.

You should be head over heels giddy about esm turning into a lingua franca for a new wave of full-stack frameworks and libraries. Spreading the use of esm far wider than before. And as a central pillar and vital part of the ecosystem collaborate to improve the quality of packages on your registry.

I really hope the current feedback presented here isn't representative of npm inc/github/microsoft as a whole and that something productive can come from this.

to be clear, "can be executed vanilla" is not an objective, or at all a universally desirable, value.

@brillout

@brillout FWIW this repository is for the npm CLI, specifically - not the npm registry (the repo being for the CLI is noted in the README). I believe registry feedback is supposed to go in npm/feedback.

Wanting to note this since you specifically stated that your goal is registry changes, and regardless of whether or not I agree with them I want to make sure you're able to get your feedback into the right venue to be heard by the correct people.

@benmccann

I don't think option 3 has to cause any fracturing. It could be implemented as a shared module

@benmccann I'm pretty sure that if you tried to get all three package managers to agree on a single implementation of things, you'll never see this feature ship. Yes, this is technically possible. However, practically/socially... much less so.

@dominikg

I disagree with you that what i said is based on a fallacy of composition but arguing about that isn't going to add any value here so please let's agree to disagree here.

@dominikg If you're not willing to discuss or back up a point you made, it probably shouldn't be made. However, I'll not engage on the topic further as long as it's not used any further as a justification for this proposal, per your request 👍🏻

This has nothing to do with the dynamic typed nature of js, so i don't understand why that is being brought into the discussion.

Because I've spent a decent amount of my career looking at / working with package registry products and very often "easy" solutions are not actually easy for registries of dynamically typed languages. I've seen what you're talking about for static language registries multiple times but have yet to see it in a production dynamic language registry. If I'm missing examples - you stated there are many - in dynamic language registries I'd love to learn about them.

Sharing the validation logic can be done, which solves the concerns about being agnostic. I am thankful you care about that, but at the same time you should acknowledge that node.js bundling npm is indeed preferential treatment and has lead to your position that the npm registry and client are by far the most used in the js ecosystem and as a result applying validation at that level is the most effective.

Yarn and pnpm are currently available via corepack in Node.js.

Last I saw, Yarn had non-trivial double digit representation (IIRC it was around 40%) in package manager usage according to npm registry statistics that were made public (this was some time ago, but I don't think it'd be under double digits at this point).

I agree with registry (IIRC it's 99%+), but disagree that moving logic like this into the registry is a good idea. Specifically, as was pointed out earlier, this is something npm explicitly doesn't do. Changing that fundamentally changes the scope of what npm is/does and that has a lot of ecosystem-wide repercussions that IMO are not positive.

Considering npm has added first-class support for types field in the past, both for the registry and the CLI:

RFC: Adding types information to the Package JSON in the registry #126
feat: add types lookup read-package-json#103 (read-package-json is used by npm publish)

I believe it's reasonable for us to make a similar argument that npm could have better support for Node.js ESM.
This could be within the scope of npm.

@brillout

@brillout FWIW this repository is for the npm CLI, specifically - not the npm registry (the repo being for the CLI is noted in the README). I believe registry feedback is supposed to go in npm/feedback.

@bnb Done, I reposted it here: https://github.com/npm/feedback/discussions/862 Validate packages before publishing them.

In the near future someone will develop another registry, an alternative to npm like yarn or pnpm is for npm cli. At npm cli they didn't get going until someone made up an alternative product, There the damage is already done.

All users will switch to this new registry because they don't bother if it's server or cli and the packages from new-validating-npm.com just work for them as easy as 1-2-3. The packages there will be properly validated and tagged "Works in ..." and "Exports provided ESM/....".

The man you work for at npm, Bill Gates, doesn't like one thing at all: alternative products and competitors. His Interest from day 1 is "developers, developers, developers"! Because developers create the applications that make his Systems valuable.

When Bill Gates will ask the quesition why he has to spend another billion to also buy the competitor new-validating-npm.com someone will point him to this thread. And he will see the stance: write something into some feedback channel and don't bother me further because we are cli and server is someone else in our company. What will he think?

Navigation Menu

Repository navigation

[RRFC] Validate ESM before npm publish #665

Motivation ("The Why")

Example

How

Current Behaviour

Desired Behaviour

References

18 remaining items

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

[RRFC] Validate ESM before npm publish #665

Description

Motivation ("The Why")

Example

How

Current Behaviour

Desired Behaviour

References

Activity

ljharb commented on Jan 3, 2023

brillout commented on Jan 3, 2023

ljharb commented on Jan 3, 2023

brillout commented on Jan 3, 2023

ljharb commented on Jan 3, 2023

brillout commented on Jan 3, 2023

ljharb commented on Jan 3, 2023

bnb commented on Jan 3, 2023

brillout commented on Jan 3, 2023

ljharb commented on Jan 3, 2023

ruyadorno commented on Jan 3, 2023

brillout commented on Jan 3, 2023

brillout commented on Jan 3, 2023

benmccann commented on Jan 4, 2023

anonrig commented on Jan 4, 2023

18 remaining items

brillout commented on Jan 8, 2023

ljharb commented on Jan 8, 2023

benmccann commented on Jan 9, 2023

stalkerg commented on Jan 9, 2023

ljharb commented on Jan 9, 2023

dominikg commented on Jan 9, 2023

ljharb commented on Jan 9, 2023

bnb commented on Jan 9, 2023

bnb commented on Jan 9, 2023

bnb commented on Jan 9, 2023

haoqunjiang commented on Jan 12, 2023

redbar0n commented on Feb 1, 2023

RobIsHere commented on Jun 24, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions