-
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
Make TypeScript itself ESM-only, made possible by require(ESM) #58419
base: main
Are you sure you want to change the base?
Conversation
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @mjbvz, @zkat, and @joj for you. Feel free to loop in other consumers/maintainers if necessary. |
9a1860f
to
42acb3b
Compare
This has been rebased now that we're using extensions on main, condensing this down to a pretty small change (though there's still stuff not working). |
bed73cc
to
739c9bf
Compare
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
There are still a bunch of TODOs, but this is actually green! Wow! |
18d1d83
to
423f584
Compare
nodejs/node#52762 was merged a bit ago; I should be able to retarget this PR at nightly. |
7c70c64
to
208e064
Compare
793228e
to
4c232d5
Compare
@typescript-bot perf test this faster [email protected] |
Starting jobs; this comment will be updated as builds start and complete.
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Huh, where is the perf hit coming from here...? |
nodejs/node#51977 (behind
--experimental-require-module
in Node 22) enables Node to require ESM so long as that ESM does not make use of top-level await. TypeScript would only need top-level await to constructts.sys
at startup, which must be defined when the environment is detected to be Node. Via nodejs/node#52599 and nodejs/node#51977, we can synchronously access Node's built-ins, and therefore can offer up a TLA-free public API, enabling TypeScript to ship as ESM-only without breaking CJS users, oncerequire(ESM)
is unflagged.The bulk of this change is just the extension rewriting, which we may want to consider taking to main just to make a PR like this one clearer.
For casual onlookers, I don't think this PR is going to "happen" any time soon (where "now" is currently May 2024);
require(ESM)
is new, flagged, and TS will likely need to support versions of Node until they're sufficiently EOL for us to feel comfortable dropping them. But it's incredible that this is possible.TODO:
__esModule
require(ESM)
having__esModule
. But I suspect that exportingdefault
fixes this.require
ifprocess.getBuiltinModule
is not foundmodule
condition/top-level prop?@types/diff
is typed wrong