-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
Node 20 ESM loaders cannot hook into the CommonJS loader #47880
Comments
@TomasHubelbauer does it make a difference if you run with or without |
No difference, see this branch: https://github.com/TomasHubelbauer/node-esm-loader-repro/tree/without-test |
/cc @cspotcode @nodejs/loaders |
This may be related to #47747. It is clear that the "off-thread loaders" feature seems to have introduced some significant instability, and in my opinion should probably be rolled back and put off until the next major, or at least until there is a straightforward way to allow loaders to either (a) modify My preference would be to roll back off-thread loaders in v20, and asap in v21, provide the capability for loaders to return transpiled source for commonjs modules. That would give existing transpilers plenty of time to migrate to the new approach, and also unify the esm/cjs story considerably, and provide a path to finally fully deprecating |
I don't foresee a rollback to accommodate a small subset of use-cases that happens to affect you. I do think enabling loaders to supply This change was 7 months in the making, and was well publicised. I am very content to facilitate and support transition. If an author needs time, inform me/us. To my knowledge, we received no requests for such, and during the time, several packages were updated. I believe the issue here is a known bug which has a simple workaround. The bug as I understand it should indeed be addressed, and we plan to do. As a side note, I am personally very offended by this cavalier and callous suggestion. I made every effort to forewarn and collaborate with package authors (I specifically tagged major packages), and in several cases, made the compatibility updates for them. We had discussed this "privately", and I suggested the resolution of enabling loaders supplying commonjs source, so I do not appreciate this public sucker-punch. |
No. The Loaders API is experimental. Breaking changes are expected. The intention is to add support for the The team working on this part of the codebase has very limited resources. If you’d like to help, your contribution would be most welcome. |
An immediate solution is to run the |
@GeoffreyBooth yes, that is a workaround for users. But more likely, they'll just be annoyed that node 20 mysteriously broke their builds. |
"Breaking changes", in the sense of changes that require consumers to adjust their consuming code, as in a break from the past API to a new thing, yes. "Just broken, without any added benefit," no. That's not what "breaking changes" means, and "experimental" is not a license to ship a broken program. The purpose of an "experimental" API is to gain information. It has done that, successfully. The information coming back from users is "this is a change that, on its own, without providing any other way to transpile CJS, makes the loaders API worse for users". The right thing to do is to take that information and act accordingly.
Great! I agree this is the correct goal.
All the more reason to revert it in v20, and continue the work in v21, in the hopes of shipping a more complete implementation in the future, without the burden of having to keep repeating this conversation for the duration of v20's support lifetime.
I would be happy to. Who is this team? What's the best way to engage with them? This is a blocker for a project I currently care a lot about, so I'm motivated to see it addressed. I am willing and able to dive in to solve the problem. But I'm not super eager to waste time on political wrangling or bikeshedding. If getting CJS transpilation into v20 is not going to be feasible in the short term, and reverting the off thread loading is out of the question, then I'll just drop support for v20 until it's taken care of. |
It did not appear in any v19 release. I don't think it was as well publicized as you think. It has taken the vast majority of TypeScript users by surprise. |
@JakobJingleheimer It was not my intent for this to be a "public sucker punch". I'm coming at this just looking at the state of the code, and what it means for consumers. This is extremely not personal. The fact is, this broke the ts-node esm loader, which is a really common way to use TypeScript. It's not a small niche use case that just happens to affect me. Please come to the TypeScript discord and hang out for a few days, I guarantee you'll see someone coming in there to complain that ts-node doesn't work on node 20. My suggestion of a rollback is not a cavalier suggestion or coming from any sort of animosity or judgement. This is normal OSS software dev stuff. It's an experimental API, and sometimes the result of an experiment is "oops, need to back up and try that a different way". There's no shame in it, and it doesn't reflect poorly on anyone who worked on the change. |
Ah! It looks like there is a way to make this work, using the So then the solution appears to be to set up require.extensions in that method, instead of just in the loader itself? That definitely changes my opinion on rolling back, but I do think it could be more obvious (assuming there isn't some reason why this is a terrible idea, in which case, I would very much appreciate if someone could please set me straight!) |
This team is the folks at @nodejs/loaders, and they’re on this thread and on the OpenJS Slack at
Great! There’s already unanimous agreement that we want a Note that it’s also on our to-do list to remove |
As long as it's not removed before the ability to transpile cjs in the loader arrives, or removed in such a way that its presence causes problems, I think that's fine. Such is life with experimental features, crufty workarounds sometime have to stay for a while. |
|
I would be careful about defining experimental that way. When something is marked experimental there should be no promise it doesn't just disappear at any time or change completely. It makes it very difficult to iterate and evolve an experimental feature to something that works well if we don't have the flexibility to make incompatible changes. That having been said, it is a difficult case when there is no non-experimental alternative. I think we need to be clearer when features like this are introduced that they are not ready for production use and that users should expect that it could change at any time until we state otherwise. Relying on loaders while they are still very much experimental was a mistake. Likely it's partly our own fault for not being clear enough about stability expectations. Also probably should have stuck to only have I've had discussions before that we should probably have a stability index step between "experimental" and "stable" to indicate that something is not at risk of change at any time but not proven enough to call "stable" yet. Possibly worth bringing up again. 🤔 |
Support for
That actually landed: #46100. We just haven’t gotten around to updating all the various sections of the docs to define currently experimental features according to the new scale. If you have time to update the sections you know well, PRs would be greatly appreciated. |
Ok, but... this is not really a case where a platform can reasonably take a hard line stance in general. Either no one ever uses the experimental features "for real", as you suggest here, and the node never gets proper play testing (so why bother shipping them in the first place), or some significant projects do put them through their paces, which requires actually using them in some load-bearing ways, in which case there's bound to be grumbling if they're broken capriciously or without alternatives. (And if capricious breakage happens frequently, no one will trust that it's safe to even try the experimental feature for anything other than toy use cases, and you're back to not getting the play testing the project needs in order to ship something really great.) The best approach is very much a case-by-case "it depends". If no one is using the feature, ok, maybe the experiment showed it's not useful, so go ahead and scrap it break it whatever. If people are using it, well, it's marked experimental, so yes you can probably get away with making more aggressive changes than something marked "stable", and while those changes may involve significant changes downstream, they shouldn't make it impossible to support the use cases people are using them for. In that case, there has to be a path forward, and some way to maintain backwards compatibility, even if involves a bunch of crufty version detection. I think everyone using loaders in a serious way right now fully understands that they're experimental, and likely to change. We've all got piles of glue code to make our libs work in the various versions of the loaders API, that's fine. My initial reaction to the off-thread loaders was based on not realizing that it would be possible to accomplish on-thread behaviors using globalPreload. Which, I still maintain would have been a bad call, but turns out I was wrong and that wasn't the case. I think maybe that workaround could be made more obvious in the docs and changelogs, and it would have been nice if it'd shipped first in an odd-number release series to catch that disruption in an unstable version, but of course sometimes it's hard to predict what's going to be an issue ahead of releasing it into the wild. And since it's a breaking change, I can see how shipping it in v19 might've been worse, and waiting until v21 might feel like it's too much of a delay. Hard stuff is hard. |
This comment was marked as spam.
This comment was marked as spam.
Should we close this issue now that ESM loader can load CJS (allowing to shortcutting the CJS loader (almost) entirely), and |
I think it’s safe to close this, and a new issue can be opened for whatever problems there are with the new approach. As in, I don’t think we’ve completely satisfied the original ask because Fastify needs an update to use the new APIs, but I would assume that once it’s updated then the use case should be satisfied; and if it’s not, a new issue should be opened. |
Version
20.1.0
Platform
Darwin Tomass-MBP-2.netis.cc 22.4.0 Darwin Kernel Version 22.4.0: Mon Mar 6 21:00:17 PST 2023; root:xnu-8796.101.5~3/RELEASE_X86_64 x86_64
Subsystem
node --loader
What steps will reproduce the bug?
Follow the steps in my repro here: https://github.com/TomasHubelbauer/node-esm-loader-repro
Copied here for posterity.
Node 20:
nvm install 20
to install Node 20node --version
to ensure Node version (I get 20.1.0)npm install
to install dependenciesnpm run test
to run thehealth.test.ts
scriptNotice the test fails and Fastify's
autoload
is seemingly not aware of the--loader
option and attempts to loadroutes/health.ts
without TypeScript toJavaScript conversion via
ts-node/esm
.Node 19:
nvm install 19
to install Node 20node --version
to ensure Node version (I get 19.9.0)npm install
to install dependenciesnpm run test
to run thehealth.test.ts
scriptNotice the test passes and Fastify's
autoload
is inherit the--loader
optionand uses the
ts-node/esm
loader successfully to auto-loadroutes/health.ts
.How often does it reproduce? Is there a required condition?
Node 20: every time
Node 10: never
What is the expected behavior? Why is that the expected behavior?
Follow Node 19's behavior of using
--loader
to transpile TypeScript to JavaScript on the fly usingts-node/esm
.What do you see instead?
An error that is the result of the loader not being applied.
Additional information
No response
The text was updated successfully, but these errors were encountered: