-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Problems with Node.js --experimental-detect-module
#56678
Comments
I'd in favor of that, even if we end up using one of the other options to workaround that issue, or if Node.js never makes |
Node also encourages everyone to always include a |
That’s something we might consider for a user’s project package.json, but the real issue for us is the thousands of existing npm packages that have no |
As Node.js tries to move forward and navigate the migration to ESM, wonder if any large-scale ecosystem projects would help. First ideas just off the top of my head to motivate conversation, of course each idea with downsides / ownership concerns: 1) Publish package details1a) npm does a 1-time analysis of all 2.6 million packages to find out the module format, using the same heuristics Node.js does with the 1a-1c could also be taken over by a different organization eg. DefinitelyTyped / Microsoft in case the value to npm wouldn't be high enough. 2) Serve
|
Node asked
|
For what it’s worth, within Node I implemented a Yes this would be a performance penalty, just like detection itself is a performance penalty, which is why we encourage everyone (especially package authors) to include |
I experimented with ways to mitigate the performance penalty of reading and analyzing additional JavaScript files. We have two limitations that take all the good options off the table: (1) our API is synchronous, so parallelizing the work within the same process is not practical, and (2) file I/O is delegated through a host object provided via the JavaScript API (this is what lets us read from unsaved editor files in TS Server, for example), so calling into another process (whether native code or parallelized JS) that performs file system operations is not possible. |
@andrewbranch I know you likely don't want to be implying Node.js directions here, but I'd be interested to hear if you feel this motivates Node.js not unflagging this feature in future? |
It’s complicated. I keep waffling between two plausible perspectives. On one hand, if we think about think about the scenarios that are most likely to be problems for TypeScript in an unflagged future, those are scenarios that are currently problems for Node.js and will be fixed by unflagging this behavior—loading “unmarked” ESM from packages that included it for bundlers, for example. Meanwhile, TypeScript’s declaration files being ambiguous is a problem of TypeScript’s own creation. Node.js is doing right by their users, and it’s on TypeScript to solve their side of things. On the other hand, given my present feedback that TypeScript won’t be able to solve our side of things to the degree that we would hope, Node.js probably needs to adjust their expectations for how much unflagging I think a good analysis here would require a better understanding of the most common consequences for users. I’ve laid out a theory of how things could go wrong, but I’m not yet sure how often we would see problems and how bad they would be. I’m curious what other feedback about this Node.js has received, and whether they are still leaning towards unflagging, and on what kind of timeline? I should also point out that because our issues center around declaration files, node_modules packages are a much bigger problem for us than local project files, and especially the loose |
We haven’t received much. There’s really only one open issue, nodejs/node#50917, which involves the edge case of non-ESM-related parse errors in a file. The “try again as ESM” behavior is triggered by an ESM-related parse error, but if V8 errors on something else before getting to the first Once that’s resolved, unless anything else comes up then it’s really just a question of how much more time we want to wait before considering this having been out in the wild long enough to unflag it. Optimistically the earliest that could happen would be in the next few months, sometime near the end of the 21.x line so that we get maximum feedback before 22.0.0 in April. Or we could unflag it as part of 22.0.0, though it doesn’t necessarily need to wait until then as it’s not a semver-major change. And of course it could always happen later, in the 22.x line or beyond, especially if issues are reported. I think the fundamental thing here is that Node has accepted (or will be accepting, if we unflag this) a conditional performance hit in the name of usability. TypeScript could do the same: that’s the first option in the above list of potential solutions, where |
It’s honestly amazing how often this meme is relevant to the dev scene in general, especially JS. Tech stacks become so intertwined that everyone starts looking at what everyone else is doing for direction, and then before you know it, bam, Spider-Man meme happens. |
I don't think this would be an option unfortunately, since having packages work locally then break on publish and install would be a really bad footgun. It's plausible we could unflag in 22 as @GeoffreyBooth describes. So if you do have any strong arguments against that now would be the time to mention I think. |
"not unflagging" still seems like the best option here; the usability isn't worth the risks. |
I momentarily forgot about the footgun of a package working during local development and breaking when installed into node_modules. I agree that’s not worth it. My instinct is that there are non-performance reasons why relying on JavaScript files to resolve declaration file ambiguity is unappealing, but so far they’re mostly rooted in principle and history. @DanielRosenwasser and I discussed this recently and he felt the same way. Maybe he can weigh in on how much that approach feels on or off the table. |
This isn’t really an issue yet, is it? Like until someone publishes a package where they rely on detection to work—so they put ESM syntax in .js files and leave out the And it’ll be a long time before anyone publishes a package that relies on detection without getting complaints from users, since any users installing that package in Node <= 21 would try to run it as CommonJS and it would error for them. So it’s probably safe the vast majority of the time for TypeScript to assume that ambiguous type definition files refer to CommonJS sources. If TypeScript changes the type definition files to be unambiguous soon, then by the time the day comes that someone publishes a “relies on detection” package that runs in most of the versions of Node active by then, that person will likely also be using a future version of TypeScript that outputs an unambiguous type definition file. There are several “most of the time” and “vast majority” qualifiers in the above, though. It’s definitely possible for someone to slip through the cracks of the edge cases and publish a “relies on detection” package that can’t run in Node < 21 but for whatever reason they generated type definition files with TypeScript 4. That said, it’s always possible for someone to publish incorrect types for a package. I guess the question is what your tolerance level is for this kind of thing. People publish broken packages all the time, that fail in certain versions of Node or certain runtimes or bundlers, so it’s perhaps an acceptable edge case to assume that ambiguous type definition files mean CommonJS and going forward they need to be unambiguous. |
I don't think this is an issue about this new option, but rather about TS in general. Specifically we have to do stuff like this in fastify: module.exports = myfn
module.exports.default = myfn
module.exports.myfn This needs to be matched by a namespace myfn {
export const myfn: MyFn
export { myfn as default }
}
export = myfn With all of this, Note that this problem is so significant that multiple modules have to:
An alternative solution for module authoring is to https://github.com/isaacs/tshy and have both a CJS and ESM being built. I do think the new option makes it worse, mostly because it exposes most developers to the same complex DX we have to do on the module authoring side.
This is something that we might want to revisit. Can you open a separate issue about this feedback? We should not plan to unflag in 22 before the TypeScript story is sorted. However, I think simplifying the developer experience for CJS ESM TS is something we should strive for, as it's one of the major DX complaints we are hearing from Node.js users. Let's collaborate. @andrewbranch @DanielRosenwasser what behavior would work for you? How do you think we can solve this problem? |
@mcollina I would love to talk about default exports but I think it’s a separate issue to work out, pretty much unrelated to Unless I’m mistaken and those are up for discussion, we’ll need to continue to find ways of discerning the module format of files Node.js is going to load, and we can iterate on default export transpilation to try to make it more portable. But (a) I think it’s a separate issue, and (b) it’s a conversation that’s not specific to TypeScript; it would be better to discuss with Babel, esbuild, Rollup, etc. |
Could DefinitelyTyped help here? They can essentially publish new types for old/already published packages, can’t they? So perhaps a solution could look something like this:
There’s obviously more complexity than just this, like currently DefinitelyTyped doesn’t have types for packages that include their own and that would need to change, but the broader point is that we do have a way of updating the types for already-published packages. |
Yes, DefinitelyTyped can be updated in bulk, which helps for a large number of old/stale but still-popular packages, but not for ones that ship their own types. I’m not sure we would want to use DT to add module format info for packages that do ship their own types (much less duplicate those types to DT), but Daniel and I did briefly mention the idea of shipping a Bloom filter encoding a set of popular npm packages that are definitely CommonJS. I forgot to mention it in my OP because it was one of the more out-there ideas. I’m not sure it would be a big improvement over “always assume CommonJS.” |
Here's an alternative solution, that I think would be easy for developers to understand, though it would involve many more organizations than just Node.js and TypeScript, though mainly npm.
I don't know if this is at all workable, but it's the easiest solution I can think of. |
I don't think it's workable for anyone to refuse to publish new packages that don't have "type". |
We asked |
Stepping back for a second, what if we did assume all ambiguous type definition files were just always CommonJS? Isn’t the problem limited to a potential future case where someone publishes a package with ambiguous type definition files where those packages only work when consumed by users with detection enabled (so ESM syntax and no So it’s really a question of what your tolerance is. If you truly wanted to ensure correct types 100% of the time, then I’d think you would always read the accompanying JavaScript file and not simply trust that the type definitions alongside it are correct. But that’s not what TypeScript currently does, so we’re already compromising on correctness in favor of performance at least somewhat. |
Actually to amend my previous comment slightly, what if:
This would close the edge case at the cost of performance, but only under a condition that would be rare to occur (someone relying on detection enabled by default, and who generated the type definition files using an old version of TypeScript). The vast majority of the time would be either correctly-guessed ambiguous type definition files that are CommonJS, or unambiguous type definition files. |
Node.js’s
--experimental-detect-module
flag poses a problem for TypeScript. If a future release of Node.js enables it by default, there is little we can do to model its semantics in our own module system under--module nodenext
, which could lead to a confusing developer experience for TypeScript and JavaScript authors relying ontsc
or editor language features in projects configured for Node.js.This is fundamentally a TypeScript problem, and I don’t know that the severity merits lobbying Node.js to change course. However, I still thought it was important to document and share as feedback.
The problem
When a user compiles a TypeScript file like
they can vary the module format of the output JavaScript file by changing the
--module
flag, but the output declaration file always looks likeSo, that declaration file text on its own is ambiguous. It could represent either of these JavaScript files:
or scripts in AMD or SystemJS format.
For TypeScript users who want to run their code in Node.js, it’s important that TypeScript can distinguish between these two cases, because it affects whether those users can access that module with certain types of imports, and what type should be assigned to the names imported from that module:
Currently, Node.js’s own module format detection algorithm resolves this ambiguity for us. The file extension of the declaration file tells us the file extension of its JavaScript counterpart, which tells us how Node.js will interpret that file’s module format:
.d.mts
.mjs
.d.cts
.cjs
.d.ts
.js
These constraints allow us to make a safe assumption about the format of every JavaScript file represented by a declaration file. For example, when we see a
.d.ts
file, we know it represents a.js
file whose format is determined by its package.json scope—if we find that package.json and see that it does not include a"type"
field, we assume the JavaScript file contains CommonJS syntax. If that assumption is wrong, the user’s problem is that their JavaScript file is misconfigured for usage in Node.js—they don’t have a problem with TypeScript.--experimental-detect-module
prevents us from being able to make safe assumptions about the module format of.js
files whose package.json scope does not define a"type"
. The flag makes Node.js detect the module format of those files by reading their contents, but TypeScript can’t do the same in the parallel world of declaration files due to the syntax ambiguity discussed earlier.Possible mitigations
Resolve and read JavaScript files when declaration files are ambiguous
TypeScript avoids reading, or even checking for the existence of, JavaScript files when declaration files are found first. The declaration files currently tell the compiler everything it needs to know about the presence and contents of JavaScript files, so there’s no point in spending extra memory and file I/O reading JavaScript files. If that changes, we could potentially resolve ambiguity by looking at JavaScript content. This would come with a performance penalty. Additonally, multiple team members expressed discomfort at the prospect of giving up the invariant of declaration files being ultimate sources of truth for the compiler. As things stand now, this is the mitigation we’re least likely to pursue.
Eliminate declaration file ambiguity going forward
We could begin changing our declaration file emit format to ensure that module formats are not ambiguous in the future. This is something I strongly advocate for, but it isn’t a solution on its own, because so many existing packages are already ambiguous, and many will never update.
Assume ambiguous files are CommonJS (i.e., do nothing)
If Node.js made
--experimental-detect-module
the default in the future, we would advocate strongly that all packages define a"type"
in their root package.json to avoid ambiguity. (We would likely consider mandating this when compiling local projects under--module nodenext
.) We could rely on this becoming best practice for new and actively maintained packages, and assume that packages lacking a"type"
field were published before--experimental-detect-module
, when such a lack of a"type"
implied CommonJS.Type imports of ambiguous files as permissively as possible
It might be possible to do some type system trickery to make something like this work:
This would be an attempt to get out of the way and never issue a false positive error, at the expense of missing some real errors.
Allow users to assert the format of ambiguous imports
In combination with some other behavior as the default, we could allow users to resolve the ambiguity themselves, perhaps with an import attribute or some central configuration.
The text was updated successfully, but these errors were encountered: