"Annotate" exported object to fix named / namespace imports of our API in Node ESM #57133
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 485ca88. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the regular perf test suite on this PR at 485ca88. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
This feels like it should be considered a bug in Node. IMO if it's going to error on named imports because it doesn't understand how |
I think I over simplified; |
|
Even so, that’s still a pretty big footgun. Anyone successfully doing a wildcard import from a CJS module probably expects to be able to do more than just |
|
One problem with this approach is that live bindings don't actually work if you use named or namespace imports from native ESM. I'm not totally sure why, as my understanding was that they worked in esbuild thanks to all of the getters and such. I can reproduce exactly the same behavior using esbuild's CJS output, so maybe this isn't a deal breaker and I'm misunderstanding things (live bindings work for the default export). Node also says that they don't update: https://nodejs.org/api/esm.html#:~:text=Live%20binding%20updates%20or%20new%20exports%20added%20to%20module.exports%20are%20not%20detected%20for%20these%20named%20exports. It's not like we have any actual bindings that update after the fact, besides say, sys, which is our only mutable export for "reasons". The default export is definitely a better thing to be doing at the user level, though, but I'm not sure how to statically force users of the TS API to do so (and people definitely use a star import more often than a default). |
|
During the design meeting, @andrewbranch pointed to #54018, which funnily contains the same explanation I independently described (which I am going to place on the Somerton Scale as "subconscious appropriation" 😄) |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at b1acdfa. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
Oof, this breaks monaco. |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at c798c02. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 66ee8fe. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
Alright, it's fixed now. Arguably monaco is doing the wrong thing in their build by using |
| @@ -2749,6 +2749,5 @@ export function isNodeLikeSystem(): boolean { | |||
| // and definitely exist. | |||
| return typeof process !== "undefined" | |||
| && !!process.nextTick | |||
| && !(process as any).browser | |||
| && typeof module === "object"; | |||
There was a problem hiding this comment.
In the new version, module is a parameter and is always provided. This check would no longer do anything. I'm not sure that it really matters that we don't have this.
There was a problem hiding this comment.
Actually, I'm going to switch this back to the same definition it used to be, e.g. checking for require. The comment I left above about require stopped being correct at some point as I refined our output bundle format to rewrite require temporarily.
There was a problem hiding this comment.
(I don't believe this will end up mattering in practice, but that's why we merge these sorts of things early in the cycle...)
Our current API bundle looks like this:
cjs-module-lexer cannot understand this. As such, this code fails:
This has been the case with TypeScript for the entire time that Node's ESM support has been around. Even before 5.0, it didn't work since our exported object was still complicated.
However, there's a footgun; this code will not error:
In TypeScript code, we'll emit helpers that fix this up. But in native ESM, this will only be an object with a
defaultproperty! You'll writets.createSourceFile, and it'll be undefined and fail later.To fix this, we need to make
cjs-module-lexeraware of our exports. When emittingcjs,esbuilduses a trick to "declare" the names to Node:This is dead code, but Node will still read the names.
If we do this same thing at the bottom of our custom bundle, we can also get named imports working. Unfortunately, there's no static way within esbuild for us to know the names via the API (likely, evanw/esbuild#3110 or evanw/esbuild#3281; I also filed evanw/esbuild#3607), so we have to import the bundle and grab the names. But, once we have them, we can tack the output onto the bundle.To fix this, the PR now instead emits CJS, then uses the header/footer (instead of just a footer + IIFE) to do something like UMD. This lets us use esbuild to make the annotations.
This now means that both of these imports work!
Fixes #56366
Related: #54018