-
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
Make tsserver and typingsInstaller thin wrappers around public API #55326
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
d2458e3
to
1aa4d3a
Compare
@typescript-bot perf test this |
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 |
@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: |
No perf impact, nice. (Was not expecting any, but still.) |
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.
Should we do a const {...} = ts.server;
at the top of the changed files, to keep the diff minimal and the accesses more static? Though I guess that won't work for types and we never added support for import {...} = ts.server
iirc.
Sure, let me give that a try, though I think most of these are actually types that change? Not sure if I can named import using an |
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.
Either way this is fine, it'd probably just be nice if we didn't have to do the long ts.server
everywhere in these files and could retain the short direct references.
I couldn't figure out how to get named imports from a nested object like that, but was able to do something slightly different by reexporting the namespaces through the older files, which keeps the diff minimal. This won't really work for ESM but that's another day. |
@typescript-bot perf test this |
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 |
@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: |
I can see from the benchmark that the last commit made tsserver 6% slower to start. I confirmed that locally too. Probably the extra copy of the server object that gets made. I'm going to revert it and stick with the version that uses |
This reverts commit 1e5fd9d.
If you check out the tsconfigs for
tsserver
andtypescript
(previouslytsserverlibrary
), you'll note that they're the same!typescript
is just a plain reexport for public use, meaning thattsserver
is a strict superset of the public API and could be implemented as a bundle which just imports the API. We already do this for vscode.dev, which adds a web host;tsserver
is what adds the node host. The same thing is true fortypingsInstaller
as well; we exported it in the public API in #53394.This PR instead makes those projects import from the public API project. Then at bundle time, this import is replaced with a reference to a local
./typescript.js
. (In--no-bundle
, this isn't done, and the original import still works.)After this, we're down to only having copies of our code in
typescript.js
andtsc.js
, where the latter is a significantly different bundle for performance reasons.Package size report
Overall package size
Files
lib/tsserver.js
lib/typingsInstaller.js