-
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
Add new option "libReplacement" #60829
base: main
Are you sure you want to change the base?
Conversation
@typescript-bot test it |
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. |
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
I guess nobody in our extended test suite uses this feature, or at least, their use of it does not actually "help" anything. Which all is a good thing. |
I am tentatively marking this as ready for review; it doesn't really seem like anyone is using lib replacement at all in our extended suite, and this is IMO a good optimization which avoids setting up a load of failed resolution watchers. But, maybe this just needs to wait for 6.0. The only thing this PR is probably missing are unit tests / some verification of the options I set on it, which I believe are probably wrong (should be more than |
bd74a31
to
a81cb69
Compare
@typescript-bot perf test tsserver-only |
Consensus in the design meeting was to default this to "true" (do lib replacement) until 6.0 where we default it to "false" (don't do lib replacement). Also, name can be bikeshedded. |
@jakebailey Here they are:
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@@ -0,0 +1 @@ | |||
[] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compare to tests/baselines/reference/libReplacement(libreplacement=true).trace.json
below, which has 1003 lines of traces for an empty file with lib=esnext
.
@typescript-bot test it |
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
bd59e00
to
c792dba
Compare
Lib replacement (https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-5.html#supporting-lib-from-node_modules) allows specially-named packages in
node_modules
to replace TypeScript's bundledlib.d.ts
files. This feature is always on, so we always perform resolution to look for it in case it exists. This can have a negative perf impact; it's extra watchers, extra CPU time, extra IO, etc.This PR adds a new compiler option
libReplacement
to control the feature; by default, it's enabled. In TS 6.0, it will be disabled by default.