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

Make Node.js more permissive when libraries ship invalid ESM code #46074

Closed
brillout opened this issue Jan 3, 2023 · 14 comments
Closed

Make Node.js more permissive when libraries ship invalid ESM code #46074

brillout opened this issue Jan 3, 2023 · 14 comments
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem.

Comments

@brillout
Copy link
Contributor

brillout commented Jan 3, 2023

What is the problem this feature will solve?

I'm the author of vite-plugin-ssr and a lot of users are having problems because many libraries ship invalid ESM code, which makes Node.js choke.

(The reason why many libraries ship erronous ESM code is because frameworks like Next.js bundle the server-side code of libraries. Where Next.js is permissive and is able to process invalid ESM code, Node.js is strict and throws an error upon invalid ESM code.)

More information at https://vite-plugin-ssr.com/broken-npm-package#solution.

This is a concern not only for vite-plugin-ssr, but also for the whole Vite ecosystem such as Nuxt and SvelteKit (see SvelteKit FAQ - How do I fix the error I'm getting trying to include a package? which is, I quote, "issues most commonly affecting SvelteKit users").

The situation is quite bad and the feedback I get from users is quite vivid, to say the least.

While one could say that it's "Next.js's fault of being permissive" (webpack more precisely speaking), the situation is what it is and it's not going to change anytime soon. So I propose following practical solution.

What is the feature you are proposing to solve the problem?

Make Node.js more permissive.

For example, 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.

I'd make Node.js permissive only for npm packages (i.e. code living in node_modules), while being more strict for the user land.

What alternatives have you considered?

[Edit] An alternative is to make npm validate packages before they are published: npm/rfcs#665.

@brillout brillout added the feature request Issues that request new features to be added to Node.js. label Jan 3, 2023
@timneutkens
Copy link

timneutkens commented Jan 3, 2023

I understand that it's all "Next.js's fault"

I strongly disagree with the kind of framing you're trying to do here.

Next.js has been around for over 6 years at this point and 6 years ago the ESM space looked significantly different from today. Over time webpack improved ESM support and added support for strict ESM resolving.

We even shipped that initially but found that the overall ecosystem wasn't ready for it through a large amount of issues/feedback probably similar to what you've been receiving. We've always favored backwards compatibility so that the surface of breaking existing applications is small when upgrading between major versions.

Many of these packages were published even before Next.js even existed or became a popular tool of choice.

To further clarify: npm packages that are opted into ESM "type": "module" are evaluated under the same strict ESM resolving that Node.js uses.

@brillout
Copy link
Contributor Author

brillout commented Jan 3, 2023

@timneutkens I meant no offense, and apologies in case you took it personally (I didn't mean to). I'm just speaking from the perspective of someone implementing a strict implementation (here the Node.js team) who may think "well, we've done a correct implementation and the permissive implementation should fix itself".

I actually don't have any "problem" with Next.js/webpack being permissive here. There are other areas about Next.js I've a problelm with, but Next.js being permissive about ESM isn't one of them 😉.

As things stand today, and regardless of why it came to be, the only option I see moving forward is for Node.js to become more permissive.

@timneutkens
Copy link

The reason why many libraries ship erronous ESM code is because frameworks like Next.js
I understand that it's all "Next.js's fault"

I assume you're going to remove these mentions from the issue as they're both wrong?

As said Next.js doesn't even handle packages that publish ESM any different and as such guides you towards publishing correct ESM packages while interop with the larger ecosystem is kept.

@brillout
Copy link
Contributor Author

brillout commented Jan 3, 2023

@timneutkens The first sentence is actually correct (feel free to PM me if you want me to elaborate), and I've just edited the second sentence. Let's make this ticket about the technical aspects and not something personal. I truly mean no offense and my sole intention is to be constructive.

@timneutkens
Copy link

There is nothing personal in this thread. You're trying to push two narratives that are untrue:

  • That I take this personal. I don't.
  • That Next.js is causing packages published as fake esm.

Packages are not being published to npm with non-standard ESM because Next.js exists. Packages published to npm using "type": "module" are interpreted the same way by Next.js and Node.js.

The edits that you've made clearly show you're trying to narrate that this is a problem Next.js introduced which is not the case as pointed out in my earlier replies.

I've just edited the second sentence

You've edited it to "one can argue" which speaks for itself that you do not want to remove the sentence that you know is wrong.

@brillout
Copy link
Contributor Author

brillout commented Jan 3, 2023

Discussing the reasons why npm packages are being published with invalid ESM isn't the goal of this ticket. (We can discuss this if you want, but I'd suggest to PM me or open another ticket instead.)

The goal here is how we can make Node.js being able to cope with the many npm packages that contain invalid ESM.

@timneutkens
Copy link

Discussing the reasons why npm packages are being published with invalid ESM isn't the goal of this ticket.

If the that is not your goal then remove the untrue mentions of Next.js because as you're pointing it out it's not relevant to the ticket.

@brillout
Copy link
Contributor Author

brillout commented Jan 3, 2023

(It's untrue from your perspective, but I do believe it to be true and that's why I'm keeping it. I want to keep it to provide a little bit of insights of why there are npm packages out there with invalid ESM. I just PM'd you about why I believe Next.js plays a role in the current situation.)

@timneutkens
Copy link

I see why you wanted to dm this as it proves even further that it's not a problem with Next.js but the larger ecosystem.

This is what was shared in DM: aws-amplify/amplify-ui#3155

Looking at the package it's not marked as ESM, there is no "type": "module" and it uses the module field instead, so it's published as a package from before Node.js ESM, this works fine in many frameworks that leveraging bundling / were create pre-ESM, it is not actually ESM, it's not marked to be ESM, which is similar to CommonJS in that it's handled differently than ESM.

The main difference between Next.js and Vite is that Next.js was created 6 years ago and already had an established userbase even before ESM became mainstream. Vite drew a line in the sand: All ESM. Next.js couldn't draw a line in the sand as it would break all existing applications (hundreds of thousands). Packages being published this way is not caused by Next.js, the majority of the packages you're referring to that have module instead of "type": "module" were published pre-ESM. E.g. the Amplify UI package doesn't have "type": "module" either: https://github.com/aws-amplify/amplify-ui/blob/be2c495c9de9373080328996f09d485d0819a190/packages/ui/package.json and that would be used by any framework.

@Rich-Harris
Copy link

Rich-Harris commented Jan 3, 2023

Chiming in, since SvelteKit was mentioned, to say that changing Node's behaviour would be a catastrophic decision.

In 10 Things I Regret About Node.js, Ryan Dahl specifically mentions extension-less imports. As the creator of Rollup, I can empathise: allowing modules to import each other without specifying the .js extension was a silly mistake that added untold complexity for no good reason. It makes authored code more ambiguous and less self-explanatory, makes runtimes slower, and adds a complexity tax to tooling that has compounded immeasurably over time. It was a horrible error.

It's unfortunate that people are currently shipping invalid ESM, during this transitional period when people are still getting used to it and haven't yet shaken off the bad habits that CommonJS and earlier design decisions encouraged. It's unfortunate — mea culpa — that tooling allows these errors to remain hidden. It's unfortunate, even, that VSCode defaults to omitting file extensions when automatically importing modules.

But none of those problems are best solved by making Node itself behave in a non-standard way. Interfaces are never made better by making them less explicit. It would be ironic if, as a result of this issue, we ended up with packages that work in Node but not other JavaScript runtimes, just as the community is finally maturing to the point that portable code is a reality.

@brillout
Copy link
Contributor Author

brillout commented Jan 3, 2023

@Rich-Harris While I agree with your point, I'm still left wondering: what can be done about the current situation?

The status quo is that a lot of packages make Node.js choke. The situation is untenable.

I actuallly really like that about Next.js: npm packages that are opted into ESM "type": "module" are evaluated under the same strict ESM resolving that Node.js uses. That's a neat move to foster ESM while being permissive.

I wonder if there is a way to make Node.js permissive while fostering valid ESM.

E.g. integrating something like https://publint.dev to npm while making Node.js permissive. Be liberal with what you accept and strict with what you send.

@brillout
Copy link
Contributor Author

brillout commented Jan 3, 2023

Actually, I'm realizing that if npm validates npm packages before they are being published could do the trick: npm/rfcs#665. AFAICT we wouldn't need Node.js to be permissive anymore. (Soon enough, most npm packages will have valid ESM.)

I'll be closing this ticket if no one comes up with an argument in favor of making Node.js permissive.

@GeoffreyBooth GeoffreyBooth added module Issues and PRs related to the module subsystem. esm Issues and PRs related to the ECMAScript Modules implementation. loaders Issues and PRs related to ES module loaders labels Jan 4, 2023
@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Jan 4, 2023

cc @nodejs/loaders @nodejs/modules

The short answer is that Node.js has no plans to adopt the loose “try to handle anything” attitude of many bundlers. That may be acceptable for a build tool where performance isn’t the top concern, but Node.js wants to run code as fast as possible and this means being a bit less permissive about what is acceptable. Especially since there are so many great bundlers that can convert sloppy code into performant, runnable code, we’d rather not slow down Node.js for everyone when either bundlers or loaders can handle those who want to wrote nonstandard JavaScript.

@bnoordhuis
Copy link
Member

In light of @GeoffreyBooth's comment I'll go ahead and close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. feature request Issues that request new features to be added to Node.js. loaders Issues and PRs related to ES module loaders module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants