-
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
Move tsserverlibrary.js to typescript.js, make tsserverlibrary.js a shim #55273
Move tsserverlibrary.js to typescript.js, make tsserverlibrary.js a shim #55273
Conversation
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. |
@@ -19,21 +18,6 @@ describe("unittests:: Public APIs", () => { | |||
it("should be acknowledged when they change", () => { | |||
Harness.Baseline.runBaseline(api, fileContent, { PrintDiff: true }); | |||
}); | |||
|
|||
it("should compile", () => { |
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.
We actually already test this by nature of the new public API compiler tests I added, which disable skipLibCheck to test the actual public API. This change could be pulled out into another PR but it's needed for this PR because this test assumes that our d.ts files are self-contained, which is no longer true.
Alternatively, I can just make tsserverlibrary.d.ts
a copy of typescript.d.ts
and it'd be fine.
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.
is is possible to create symlinks for tsserverlibrary.js and tsserverLibrary,d,ts to point to typescript.js and typescript.d.ts instead
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.
Right now tsserverlibrary
just reexports typescript
, it's just that it's not self contained so this one particular test breaks (but isn't needed anyway).
In terms of the package itself, we can't really use symlinks as those are not universally supported on Windows or by package managers themselves. There are ways that people store packages which can't contain symlinks at all (e.g., zips, used in yarn, for example), and it'd be a bit dicey to commit back to a release branch.
Herebyfile.mjs
Outdated
else if (typeof importScripts !== "undefined") { | ||
importScripts("./typescript.js"); | ||
} |
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.
I'm not totally sure if this is a good idea in general; I'm guessing it's possible for this line to make it into someone else's bundle which will start importing things if it happens to be a worker... but maybe not if they're correctly emulating CJS?
I'm half wondering if it'd be better to just not do this line at all and leave it as a module reexport, and just wait for feedback that says we need to do better.
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.
But, I somewhat expect that nobody is actually loading this bundle into a browser at all.
@@ -6,7 +6,7 @@ | |||
{ "path": "../compiler" }, | |||
{ "path": "../jsTyping" }, | |||
{ "path": "../services" }, | |||
{ "path": "../deprecatedCompat" } |
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.
Is this intentional to drop drepcated compat from typescript.js ?
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.
It's already in the server
tsconfig / project, since it's needed for both tsserver
and tsserverlibrary
; to make this PR I actually just deleted all of the contents of typescript
and moved tsserverlibrary
here, but git unfortunately can't show that!
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.
it should still be here since references are not transitive.
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.
They aren't? But, tsserverlibrary's been set up this way since I merged modules and has been fine; what breaks if this isn't done? If it's about the code itself, it's imported in the server
namespace so gets pulled in there.
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.
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.
Build as in typecheck or as in write out files? I guess dts emit may matter for this...
I'm surprised in that I think a lot of monorepo projects aren't just bulk including all transitive dependencies in their tsconfigs as that would get unweildy.
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.
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.
Aha! Thanks!
I'll go figure out which transitive things we're missing.
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.
I pulled this out into #55339, which I'd like to merge first.
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.
Given this isn't a regression (all I did was move tsserverlibrary
and rename it), this shouldn't block merging of this PR, right? So the above isn't exactly a prereq?
@typescript-bot pack this |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at d3fc401. 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 |
`; | ||
|
||
const lsslDtsInternal = ` | ||
import ts = require("./typescript.internal.js"); |
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.
This is a little unfortunate for anyone who is attempting to pull these internal types, e.g. byots/ts-expose-internals but the fix would be quite simple for them; just use typescript.internal.d.ts
for both.
@JoshuaKGoldberg you may beat me too this, but I think we should try including the above package in ts-eslint and see if it compiles, to make sure my new d.ts works. That and I need to also test vscode, which similarly imports this but also augments it IIRC. (I'll do that Monday) |
Actually, took me like 2 minutes to test in vscode, and it worked even with their augmentation, so it probably is fine everywhere else. |
@@ -563,7 +586,7 @@ export const watchLocal = task({ | |||
name: "watch-local", | |||
description: "Watches the full compiler and services", | |||
hiddenFromTaskList: true, | |||
dependencies: [localize, watchTsc, watchTsserver, watchServices, watchLssl, watchOtherOutputs, dts, watchSrc], | |||
dependencies: [localize, watchTsc, watchTsserver, watchServices, lssl, watchOtherOutputs, dts, watchSrc], |
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.
Last minute change; we still have to run lssl
here at least once in watch mode, so we do it just like dts
here.
Meant to ping @mjbvz too; I don't think that this will affect VS Code at all; I think both of these files are preserved in your pruned package and this probably makes the whole editor lighter! |
The thinking here is:
ProjectService
without the problems of importing typescript twice.ProjectService
intotypescript.js
is effectively moving the entire server namespace intotypescript.js
.tsserverlibrary.js
?Before this change, the two bundles loaded in roughly:
typescript.js
- 206.30ms (± 0.30%)tsserverlibrary.js
- 223.68ms (± 0.21%)So, this only adds about 20ms of extra time to importers. But, it does increase the size of
typescript.js
by 7%: https://github.com/microsoft/TypeScript/actions/runs/5814577872#summary-15764542922In the process of this, we also noticed that we can in the future:
cc @JoshuaKGoldberg @bradzacher
For #27891