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

TypeScript 4 upgrade #16101

Closed
joschect opened this issue Dec 1, 2020 · 6 comments · Fixed by #17932
Closed

TypeScript 4 upgrade #16101

joschect opened this issue Dec 1, 2020 · 6 comments · Fixed by #17932

Comments

@joschect
Copy link
Contributor

joschect commented Dec 1, 2020

Ported from hackmd

Problem description

Typescript 3.7 which builds react and react-northstar in the currently in the fluent repo is just over a year old. The current version is 4.1 (released mid-November).

In order to stay current we should invest time to upgrade the repo to Typescript 4.1.

This also unblocks setting up project references across the repo (as a follow-up project), which will provide a greatly improved dev experience when working across multiple packages.

Appetite

This is something that needs to be done to stay current with typescript and encourage partners to also modernize their own infrastructure. Previous initiatives to improve build infrastructure in the repo such as introducing project references were blocked by typescript bugs and could be tried again once the repo is upgraded to 4.0+.

This project is a good candidate for tag-teaming between time zones: work in a shared branch, and at the end of the day push your current progress and leave some notes about status/questions/next steps.

Solution

The main objective of this project is relatively straightforward that we should bump the current typescript version in the repo and get passing builds in all projects.

@fluentui/react has a hard constraint to maintain support for the typescript version for the latest release which for v8 is still 3.7.

Once we manage to get passing builds with 4.1 we would need a validation process for older typesript versions and perhaps release a beta for a partner.

The validation steps at least should be

  • Green CI for both N* and v8
  • Beta release of v8 on selected partners
    • TODO find good beta candidates with good CI
  • Beta release of N* on teams with green CI

We could also investigate libraries such as the downlevel-dts package to integrate with our build scripts to guarantee some form of backwards compatibility for older typescript versions.

In addition to upgrading TS itself, we'll need to upgrade some of our build dependencies which are specific to particular TS versions. This will definitely be the case with API Extractor and possibly also ts-jest, ts-loader, and maybe others.

Risks

The uncertainty of what is required to complete the upgrade is risk. Typescript doesn't offer backwards compatibility or follow semantic versioning, so it's easy to accidentally break partners. It also tends to provide stricter type checking each release, which is a good thing overall, but can cause migration pain (especially with the size of our codebase and complexity of some types).

This project is difficult to fully scope ahead of time and could take either a few days or a few weeks. Previous work on typescript in the repo has shown as much.

For upgrading tooling within the repo itself, some of the risks are as follows:

  • Upgrades typically start with a flood of errors which mostly have the same handful of root causes. These can be a bit time-consuming due to the volume, but straightforward.
  • After that, there are usually a few remaining tricky errors. The time to deal with these (or to determine that they're insurmountable without TS fixes) varies.
  • Sometimes there are excessive increases in either memory consumption or build time, which often require involvement from the TS team to debug. These can sometimes be worked around by tweaking our type definitions, but other times a TS fix is required.
  • Once things compile (with TS) successfully, we have to upgrade all the other tools (such as ts-jest, webpack loaders, API Extractor) to compatible versions. This may be uneventful or may reveal additional issues (including sometimes that a compatible dependency version hasn't been released yet).
    • This time, since our jest and webpack versions are both somewhat out of date, there's an additional risk that the versions of the deps which work with the new TS version won't work with our old jest/webpack versions.

Once we release, we may hear about additional issues from partners:

  • A backwards-incompatible typing change made it through, typically requiring a quick fix on our side. (This can be mitigated by adding a TS 3.7 validation step to our build and validating with a partner before merging.)
  • Something about the emitted code changed in a way that caused bundle size regressions. (Can be somewhat mitigated by validating with a partner before merging.)

There is also a risk of an extended validation period required for partners who would need support for older versions. We should not revert any TS upgrade changes if support for anything less than minimum supported version is required (currently 3.7)

Since 4.1 is quite new, it's possible it has some bugs which haven't been discovered/reported yet. It's still worth trying 4.1 first, but if we encounter any difficult issues which don't repro in 4.0, going to 4.0 for now is an acceptable fallback. (We can't go lower due to a bug in 3.9 which would substantially increase partners' bundle sizes.)

Another thing that wasn't clear from the documentation is whether we'll need to upgrade all the projects' tslib dependency to version 2.

Out of scope

The objective of this project is to replace typesript 3.7 with 4.1 and achieve backwards compatibility until 3.7 for dts files. All changes reflected should only contribute to this goal. Avoid making any other kind of build improvements until the upgrade is completed; any unnecessary changes could make this upgrade more painful.

The upgrade will be made in master only (not 7.0).

The typescript version under packages/web-components should probably stay the same since it's managed separately.

@Hotell
Copy link
Contributor

Hotell commented Jan 20, 2021

I'd suggest slightly different approach:

TL;DR:

Long version:

1. migrate TS to single version policy approach with the ability to opt out in any package (web-components only for now)

PR #16592

  • we already know that working with multiple versions/non exact versions of any dependency is quite problematic and hard to reason about.
  • rather having various devDependencies specified all around the place, having one exact version within root, thus being used by every package, is more convenient and easy to reason about. With TypeScript in particular, this significantly improves any kinds of miss matches between 3rd party tools and type checking
  • with this in place, opt-out behaviour is possible if really needed, by turning off hoisting per domain/package

Remarks:

  • opt-out 👉 yarn nohoist

2 - migrate to 3.9.7

  • bump to 3.9.7
  • use @ts-expect-error pragma in code where errors occurred
  • resolve errors via atomic approach distributed in team

3 - setup typeVersions within published packages and add downleveling to ambient definitions within build pipeline

  • as we gonna use 3.9 which added new incompatible lang feature (type imports), we should downlevel types for v7/v0 packages to no introduce Breaking changes to consumers

Remarks:

4 - migrate to 4.0.x

  • bump to 4.0.x
  • apply same steps as in 2

5 - migrate to 4.1.x

  • bump to 4.1.x
  • apply same steps as in 2

step 1 and 2 demonstrated in Draft PR -> #16544

@ecraig12345
Copy link
Member

@Hotell This proposal could make sense overall, but there are a couple issues we'll need to deal with.

1. migrate TS to single version policy approach with ability opt out

Agreed about this.

The only single-version exception today should be with the web-components package (need to check with @chrisdholt before making any changes to its TS version).

2 - migrate to 3.9.7

We were blocked from going to 3.9 by a change it made to how certain exports were emitted which caused massive increases in bundle size for consumers. @dzearing do you remember the details of this issue? I'm unfortunately also not 100% clear on whether it's been fixed in 4.1, or whether we were just hoping it would be...

  • use @ts-expect-error pragma in code where errors occurred
  • resolve errors via atomic approach distributed in team
    I already have resolutions in (WIP) Upgrade to TypeScript 4.1 #16449 for a lot of the errors. If we do end up using a distributed approach at some point, my concern is how we ensure that it actually gets done instead of just ending up with (essentially) a more dangerous version of "todos" which stick around forever.

3 - setup typeVersions within published packages and add downleveling to ambient definitions within build

There are two parts to this from my perspective. We could potentially go ahead and add this tooling while still on 3.7, and the two pieces could be done separately and/or by different people.

  • Add downleveling and typesVersions, probably using downlevel-dts (if the repo is still on 3.7, you could test by doing something like downleveling to 3.5)
  • Add tests to validate that we didn't break 3.7. Basically make a file with root imports of the relevant list of packages, then verify it builds with 3.7 (possibly in a temp folder sandbox similar to the CRA test).

Should clarify with @layershifter what promises v0 makes (if any) about TS version compatibility--it might not be an issue for them.

I also think we probably don't need to enable downleveling (at least not down to 3.7) for the new converged packages.

4 - migrate to 4.0.x

5 - migrate to 4.1.x

I could go either way on this part, but I was leaning towards attempting to do the upgrade all at once first (partly in case of bugs in 4.0 which may be fixed in 4.1) and then going back to 4.0 only if we found problems with 4.1.

@Hotell
Copy link
Contributor

Hotell commented Jan 21, 2021

The only single-version exception today should be with the web-components package

that's fine for now and already applied in my PR. As described in my proposal (opt out). Future goal would be to get it unified for everyone (maintaining costs, etc mentioned in my proposal)

which caused massive increases in bundle size for consumers.

Yeah I can see that on CI report. that's unfortunate. would love to get link to TS repo with what's the cause.

I already have resolutions in #16449 for a lot of the errors. If we do end up using a distributed approach at some point, my concern is how we ensure that it actually gets done instead of just ending up with (essentially) a more dangerous version of "todos" which stick around forever.

Well my team and owner of those code parts should be responsible in particular sprint. (even better to pair with person/team that owns codebase which is having issues. From personal experience, I don't think the approach (Im gonna fix it all) works nor scales.

There is ofc one exception to this (gradual + ts-expect-error pragma) - if that's public API -> that might mess up types. In errors that occured in my PR, no public apis are affected.

probably using downlevel-dts

👍

Add tests to validate that we didn't break 3.7. Basically make a file with root imports of the relevant list of packages, then verify it builds with 3.7 (possibly in a temp folder sandbox similar to the CRA test).

Testing the compatibility would be more complicated than just importing all components and run tsc on that file. We would need to test basically all component APIs, to not break the type contract contract (wondering if there is such a contract between our consumers at all - type checking wise). We need to discuss this in separate issue.

Out off topic:

  • this will add another maintenance burden on our shoulders, I'd recommend to communicate bumping typescript as breaking change of fluent library in the future (starting with convergence).

We could potentially go ahead and add this tooling while still on 3.7

good idea 👍, lets do it like that

I could go either way on this part, but I was leaning towards attempting to do the upgrade all at once first

Right. I'd stick with gradual approach whenever possible (although TS 3.9 is apparently a no go step because of that increased bundle size - another good reason to use babel/esbuild for transpiling in convergence initiative). As I mentioned, "all at once" ends up usually with small chance getting merged in short period if at all, especially with typescript which doesn't follow semver.

@ecraig12345
Copy link
Member

Minor update: I looked into whether upgrading to tslib@2 is required to use TS 4.1--it appears the answer is yes, but also it shouldn't cause problems for consumers on older TS versions (aside from the possibility of a minor bundle size hit from two versions of tslib).

Details

Per the tslib docs, the minimum typescript version to use tslib@2 is 3.9. So we can't and shouldn't use it until we internally upgrade to TS 3.9.

Per the tslib 2.0.0 release notes the only breaking change in version 2 is:

This release changes __exportStar and __importStar to use __createBinding which is incompatible with versions of TypeScript older than 3.9 (i.e. 3.8 and below).

Per this PR description:

TypeScript 3.9 won't work on anything older than tslib 2.0

From what I can tell (see this PR comment), the tslib version which must be used is bound to the TS version used to compile the library, not to consume it. So whenever we internally upgrade to TS 4.1, I think we should be able to update to tslib@2 at that time without any affect on consumers (besides a minor bundle size hit).

@JustSlone
Copy link
Collaborator

JustSlone commented Mar 16, 2021

Update:

  • In Progress
  • Reach out to Chris on web-components for Typescript policy bump

@ecraig12345
Copy link
Member

The main upgrade PR #17932 has been merged. Created issue #18025 to track follow-ups.

@microsoft microsoft locked as resolved and limited conversation to collaborators Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.