Skip to content
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

Can we stop shipping typescriptServices.js? #50758

Closed
DanielRosenwasser opened this issue Sep 13, 2022 · 4 comments · Fixed by #51387
Closed

Can we stop shipping typescriptServices.js? #50758

DanielRosenwasser opened this issue Sep 13, 2022 · 4 comments · Fixed by #51387
Assignees
Labels
Discussion Issues which may not have code impact Fix Available A PR has been opened for this issue

Comments

@DanielRosenwasser
Copy link
Member

typescriptServices.js is a global file that provides the language service; however:

  1. I believe most of the same functionality is exported in typescript.js and tsserverlibrary.js, just in CommonJS.
  2. Most language service consumers are just using tsserver these days anyhow.

Is there any need to continue shipping this file to npm?

Maybe these queries will help give us a few pointers as to who's using it

@jakebailey
Copy link
Member

jakebailey commented Sep 13, 2022

I believe most of the same functionality is exported in typescript.js and tsserverlibrary.js, just in CommonJS.

The two files are identical, except that they point to different source maps (which are the same, but at different paths, so still redundant). Right now, we use the fact that it's a single file via namespace merging to dynamically choose to export via module.exports (https://github.com/microsoft/TypeScript/blob/main/src/services/exportAsModule.ts) or be global (https://github.com/microsoft/TypeScript/blob/main/src/services/globalThisShim.ts).

Anyone currently using typescriptServices.js should be able to use typescript.js and see no changes.

However, post-modules, the two linked files probably aren't going to work, though I may be able to push them to the top level file instead, actually.

@fatcerberus
Copy link

The two files are identical

FWIW, I noticed this a long time ago and was very confused. I eventually just decided typescript.js must have been the “canonical” one for API consumers and used that, and the inclusion of a duplicate file by a different name was either an accident, or else some internal implementation detail of the node package and stayed away from it. I would have never guessed typescriptServices.js was meant to be the canonical one!

@jakebailey
Copy link
Member

jakebailey commented Sep 13, 2022

Looking back at the Gulpfile, it appears to have been this way for at least 6 years.

I think we could definitely remove this file and tell people to use typescript.js, given they are identical, though if we are considering that to be a breaking change, then that'd probably want to wait until 5.0, in which case it's somewhat moot given I am really hoping to transition TS to modules in that release. But, the fact that these files are identical except for the export line to me implies that I can (for now) completely ignore it for the module'd version of TS.

To clarify "somewhat moot", only if we don't bundle. We still may!

@andrewbranch andrewbranch added the Discussion Issues which may not have code impact label Sep 13, 2022
@jakebailey
Copy link
Member

jakebailey commented Sep 26, 2022

Following up on this since I already have typescriptServices removed on my module branch (makes the build simpler).

I've done some looking, and it looks like typescriptServices is the main file used by VS Code and monaco. The latter is interesting because it looks like they use AMD wrappers (and the same thing is mirrored in the technically-broken-and-not-used createPlaygroundBuild script in our repo), so they will probably need some logic to choose the right path if the old one doesn't exist.

Our API tests also assume that the types come from typescriptServices via a hacky way of getting things, but I already have to redo that for the module branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Issues which may not have code impact Fix Available A PR has been opened for this issue
Projects
None yet
5 participants