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

repo: upgrade to TypeScript 4 #10355

Merged
merged 1 commit into from
Feb 8, 2022
Merged

repo: upgrade to TypeScript 4 #10355

merged 1 commit into from
Feb 8, 2022

Conversation

paul-marechal
Copy link
Member

@paul-marechal paul-marechal commented Oct 29, 2021

@tsmaeder I remember saying we should avoid upgrading TypeScript, but I changed my mind.

I want to upgrade in order to build slightly faster and hope to fix an issue with our current version of tsserver being quite slow on our repo. Switching to TypeScript 4.4 I couldn't notice the lag anymore.

Upgrading the TS version we use to build this repo might lead to breakage in downstream applications in the following case:

  • We start using a new TypeScript 4 construct. e.g. Named tuple elements (type SomeTuple = [a: string, b: number])
  • Downstream consumers of Theia use TypeScript 3 and it fails to parse this definition.

The solution would be for them to also upgrade their version of TypeScript in such a case. I think this is manageable.

As @msujew mentioned: TypeScript doesn't follow semantic versioning. But patch versions shouldn't bring breaking changes.

Closes #10354.

How to test

  • Building this repo should still work.
  • Once everything is built, try running a few LS commands and get a feel for the performance. It shouldn't take to long to complete. It's quite subjective...

Review checklist

Reminder for reviewers

@paul-marechal paul-marechal added contributor experience issues related to the contributor experience dependencies pull requests that update a dependency file potentially-breaking A change that might break some dependents conditionally labels Oct 29, 2021
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

The localization-manager package also has a dependency to typescript (as a runtime dependency), do you mind updating that one as well? I think we should keep them synchronized.

doc/Migration.md Outdated Show resolved Hide resolved
@tsmaeder
Copy link
Contributor

tsmaeder commented Nov 2, 2021

@paul-marechal I'm not against keeping up with typescript versions, but I'm getting a bit worried with the rate of changes we have in our build system. We don't want adopters to have to catch up every release. Would it be possible to update the tooling (which is what we want) and keep the syntax checking at 3.9?
It would be nice if we could be more predictable with such changes, such as announcing breaking updates to tooling a release in advance.

@paul-marechal
Copy link
Member Author

We don't want adopters to have to catch up every release.

The build system shouldn't impact adopters that are consuming the npm packages we are publishing, like at all?

Beside the potentially new type definition, which won't cause any issue until the moment someone makes a PR using new things from TypeScript.

For those that build the repo themselves, doing yarn alone should do everything up until the point where all packages are built and ready for use. Unless the wiring is deeply plugged into how Theia is built?

@paul-marechal
Copy link
Member Author

and keep the syntax checking at 3.9?

Not AFAIK, we'd have to keep an eye out on what goes in but that doesn't sound super robust.

@tsmaeder
Copy link
Contributor

tsmaeder commented Nov 3, 2021

The build system shouldn't impact adopters that are consuming the npm packages we are publishing, like at all?

Since we have no way to ensure that no > 4.0 constructs are used in Theia, they will effectively have to move to 4.0, as well eventually. Looking at the Typescript changelog, that might be a breaking change.

@paul-marechal
Copy link
Member Author

paul-marechal commented Nov 3, 2021

@tsmaeder Yes, indeed. Although it should be a trivial change for downstream clients, should anything break.

Last time we broke something related to TypeScript it was the compilation target and the community seem to have received it well? With the Node/Electron upgrade we might be able to bump it again btw.

Also, the TypeScript version we've been using is a year old already. I don't think it's a crazy idea to try and not lag too much behind in the future. I think Anton had suggested targeting latest for the TypeScript version and with our yarn upgrade process I was worried we would have issues. With hindsight it would actually be fine and we should fix any new compilation error as part of the upgrade process, unless it's a big breakage (unlikely).

In other projects this dependency upkeep and build system care is usually called "chore" because it is, but it's necessary for a healthy project IMO. I had the pleasure of addressing parts of the build system that hadn't changed in a year or more, and the debt was painful to pay in full. Doing it incrementally should be better.

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

Looks fine to me. Minor comments on the @ts-expect-error comments. Probably if we compiled Phosphor ourselves with a later TS version, we could get rid of those, but that doesn't seem worth it. I don't love the number of new any's that are introduced, but they point to places where we're not being as clear about our intent as we should be, and I'm fine with them for now.

The upgrade didn't seem to improve my build or code browsing experience, but I agree with @paul-marechal that these are chores that we're stuck with - we can't really plan to lag behind indefinitely, so we'll have to do this at some point. Certainly open to discussion about the schedule on which we should upgrade our tools, but this doesn't seem hasty to me.

packages/core/src/browser/shell/tab-bars.ts Outdated Show resolved Hide resolved
packages/core/src/browser/view-container.ts Outdated Show resolved Hide resolved
packages/debug/src/browser/debug-session-connection.ts Outdated Show resolved Hide resolved
Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

By the way, I can confirm that running Find All References with TypeScript 4.4.4 runs way faster in Theia now, compared to 3.9. However, the current default for the TypeScript extension seems to be 3.8, so I had to manually change it to the workspace version to verify this.

@paul-marechal
Copy link
Member Author

paul-marechal commented Nov 4, 2021

the current default for the TypeScript extension seems to be 3.8

How? I thought the extension would use what VS Code bundles I thought it would be using 4.4?

We also have the TypeScript version set to node_modules/typescript/lib in our .vscode/settings.json file, so it should pick up the locally installed one?

@msujew
Copy link
Member

msujew commented Nov 4, 2021

How? I thought the extension would use what VS Code bundles I thought it would be using 4.4?

@paul-marechal I was confused as well, but we currently use the 1.45.1 version of the typescript-language-features extension in the Theia repo, which comes bundled with TypeScript 3.8.3:

image

And you're right as well that we added the appropriate setting to use the node_modules/typescript/lib library, but there's a huge catch hidden in the docs of the extension:

The typescript.tsdk workspace setting only tells VS Code that a workspace version of TypeScript exists. To actually start using the workspace version for IntelliSense, you must run the TypeScript: Select TypeScript Version command and select the workspace version.

So the setting only tells the extension which workspace version should be used, but it doesn't tell it to use that one from the start. The user has switch manually. I just learned about that one as well.

@paul-marechal
Copy link
Member Author

paul-marechal commented Nov 4, 2021

Thanks for pointing all of this out @msujew.

VS Code will automatically detect workspace versions of TypeScript that are installed under node_modules in the root of your workspace.

This means that the typescript.tsdk setting we had is useless now, so I removed it.

edit: Turns out we can't really delete it.

.vscode/settings.json Outdated Show resolved Hide resolved
@paul-marechal
Copy link
Member Author

I rebased this PR on master and also pushed TypeScript to 4.5.

@paul-marechal paul-marechal mentioned this pull request Feb 1, 2022
1 task
@JonasHelming
Copy link
Contributor

@tsmaeder

Copy link
Member

@msujew msujew left a comment

Choose a reason for hiding this comment

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

Thanks Paul! Looks good to me, I have a small remark, but I'll approve this already 👍

dev-packages/localization-manager/package.json Outdated Show resolved Hide resolved
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

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

The changes look good to me 👍
I confirmed that

  • editing in vscode still shows markers for tslint
  • typescript functionality works (and feels quicker)
  • the application builds and starts

Copy link
Contributor

@colin-grant-work colin-grant-work left a comment

Choose a reason for hiding this comment

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

LGTM and seems to work at least as well. Excited to get better type check handling, and some day I may find the time to address the new any's :-).

@JonasHelming
Copy link
Contributor

@paul-marechal : Let's merge this!

@paul-marechal paul-marechal merged commit ea18055 into master Feb 8, 2022
@paul-marechal paul-marechal deleted the mp/ts4 branch February 8, 2022 15:17
@github-actions github-actions bot added this to the 1.23.0 milestone Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contributor experience issues related to the contributor experience dependencies pull requests that update a dependency file potentially-breaking A change that might break some dependents conditionally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript's Find All References and other requests are slow when used in this repository
6 participants