-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Tracking issue: Named imports from CJS module incorrectly allowed in nodenext #54018
Comments
It also just feels wrong for this to be declared explicitly, seeing as Node’s solution is clearly a best-effort kind of deal (hence the static analysis instead of e.g. side-channel data in |
This is quite a common issue in CJS modules, as many are not written with this rule in mind. Even something like |
IMO this merits an issue filed on the library wherever it comes up. |
Definitely. But not all of them are well maintained. 😢 |
FWIW as of #57133, the original example from TS no longer fails as I changed how we bundle to "annotate" the exports in such a way that the lexer can see the names, but that obviously doesn't help anyone else. |
I encountered a similar problem in issue nodejs/node#56304, where named exports cannot be imported. Through discussions and analysis with @aduh95, the problem should lie in how In summary, to solve the problem, either |
This issue has come up a few times, most recently at DefinitelyTyped/DefinitelyTyped#65147 (comment), and I don’t think we’ve had a canonical place to explain, discuss, or track it.
The symptom
Sometimes, when you’re writing an ES module in
--module nodenext
, you will want to import a CJS dependency, and TypeScript lets you used a named import:but Node complains:
Alternatively but equivalently, you might try to use a namespace import:
and Node gives a much less helpful error:
In either case, you find that you instead need to write a default import:
Both TypeScript and Node are happy with this, but why did TypeScript let you write the forms that crash at runtime?
The problem
Node uses cjs-module-lexer to syntactically analyze CommonJS modules without executing them in order to turn
module.exports
properties into named exports that can be imported as named (or namespace) imports by ES modules. However, syntactic analysis has its limitations, and not allmodule.exports
properties get detected on all modules. (In practice, it’s common for this to be all-or-nothing, as with thetypescript
package in its current state: it has no named exports available.)Type definitions have no way of declaring which exports/properties of a CommonJS module will be detectable by Node’s named export analysis. In other words,
typescript.d.ts
is not misrepresenting the shape of the module; it would be impossible to “fix” it for Node ESM consumers without breaking it for CJS consumers or bundlers, which typically have more relaxed interop rules. TypeScript currently assumes that all declared exports/properties of a CJS module will be detectable by Node and available as named imports. This is unsound, leading to the error in the example above.It should be noted that at least for the named import case, the crash occurs during Node’s module linking phase (as soon as the module graph is loaded—at startup, unless the affected part of the graph is isolated in a dynamic import), with a good runtime error message. While annoying to run into, it’s usually immediately diagnosable and fixable. The namespace import variation is a bit more insidious, since the import can be linked, but subsequent property accesses, which can occur later in execution, may fail.
Solutions and non-solutions
I’m putting this issue up for tracking/documentation purposes, not to advocate for a fix. But it’s worth mentioning a few ways of addressing the problem since people will ask or suggest them:
The text was updated successfully, but these errors were encountered: