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

Remove typescriptServices.{js,d.ts}, protocol.d.ts, typescript_standalone.d.ts #51026

Closed

Conversation

jakebailey
Copy link
Member

Fixes #50758, for TS 5.0.

These are files that nobody appears to use anymore, or are duplicated somewhere else. Notably, I have made all of these changes already on the module-ified TS branch, so this is a backport in prep for that (adapted for our current gulp build).

I'll try a pack this once microsoft/TypeScript-Make-Monaco-Builds#14 is in.

TODO:

@jakebailey

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@sheetalkamat
Copy link
Member

protocol.d.ts: We created this for vs and vscode as far as I remember.. are they not using it.

@jakebailey
Copy link
Member Author

Hm, so our current LKG script appears to be unable to delete files that are no longer copied, so this isn't yet a perfect test.

@jakebailey
Copy link
Member Author

Hah, so, this explains why Deno has in their "updating typescript" instructions a step that is deleting "dead" d.ts files. Our script never actually removed files that are no longer part of the build.

@jakebailey

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@typescript-bot

This comment was marked as outdated.

@jakebailey
Copy link
Member Author

Alright, seems to work now.

@jakebailey
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 3, 2022

Heya @jakebailey, I've started to run the tarball bundle task on this PR at 7ddba70. You can monitor the build here.

@jakebailey
Copy link
Member Author

My latest PR clears the lib folder (besides README) before copying into it, so that the files get deleted.

@weswigham mentioned that this might be undesirable; I can imagine that we actually do want to keep "dead" files forever, or something, so that people can continue to reference them in their old projects, but I'm not sure if that's intentional or not. If we do want this, it seems like we'd actually want to have those files in src/lib and declare that they really do exist.

For now, I can instead just change produceLKG to warn on files that don't appear to have come from any input, and then explicitly have it delete the files that this PR deletes.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 3, 2022

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/135755/artifacts?artifactName=tgz&fileId=8556FFB01472105E7DA3BE7F090EB03B311130D7046AE48C47E45E693DCB30E602&fileName=/typescript-4.9.0-insiders.20221003.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.9.0-pr-51026-13".;

@jakebailey
Copy link
Member Author

Looking at the diff when I run LKG locally, these files are deleted:

        deleted:    lib/lib.esnext.promise.d.ts
        deleted:    lib/lib.esnext.string.d.ts
        deleted:    lib/lib.esnext.weakref.d.ts

It appears as though we handle these explicitly:

["esnext.string", "lib.es2022.string.d.ts"],
["esnext.promise", "lib.es2021.promise.d.ts"],
["esnext.weakref", "lib.es2021.weakref.d.ts"]

@jakebailey
Copy link
Member Author

For reference, the new package contents: https://unpkg.com/browse/@typescript-deploys/pr-build@4.9.0-pr-51026-13/lib/

@jakebailey
Copy link
Member Author

Pulled out the part of this that can be merged early into #51061.

@jakebailey jakebailey force-pushed the remove-typescriptservices branch 2 times, most recently from 01d70f4 to bc1fcec Compare October 4, 2022 20:40
@@ -6,7 +6,7 @@

// This script does two things:
//
// - Listens to changes to the built version of TypeScript (via a filewatcher on `built/local/typescriptServices.js`)
// - Listens to changes to the built version of TypeScript (via a filewatcher on `built/local/typescript.js`)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will have to be updated to match the final state of monaco-editor. e.g. microsoft/monaco-editor#3356

@jakebailey
Copy link
Member Author

@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 11, 2022

Heya @jakebailey, I've started to run the tarball bundle task on this PR at fddcda1. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 11, 2022

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/136300/artifacts?artifactName=tgz&fileId=A1CDFF7CA07E1FF62B46AA2900D6CE02B14B5B40E082794D97D599B5C60419D502&fileName=/typescript-4.9.0-insiders.20221011.tgz"
    }
}

and then running npm install.


There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@4.9.0-pr-51026-19".;

@jakebailey
Copy link
Member Author

jakebailey commented Oct 11, 2022

protocol.d.ts: We created this for vs and vscode as far as I remember.. are they not using it.

@sheetalkamat I totally misread your comment, I didn't realize you were asking if they were using this file.

It turns out that they are using it, though indirectly: https://github.com/microsoft/vscode/blob/main/extensions/typescript-language-features/src/protocol.d.ts

I'll have to see if we can instead have them write something like:

import * as ts from "typescript/lib/tsserverlibrary"
export = ts.server.protocol;

Or something, which I think should do the same thing.

@jakebailey
Copy link
Member Author

I sent microsoft/vscode#163365 with this approach, which actually revealed a couple oops-es in the VS Code codebase.

@jakebailey
Copy link
Member Author

Going to close this; I will include this in the module conversion PR when the time comes.

@jakebailey jakebailey closed this Oct 20, 2022
@jakebailey jakebailey deleted the remove-typescriptservices branch November 7, 2022 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can we stop shipping typescriptServices.js?
3 participants