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

chore(deps): Upgrade to Typescript 5 #4323

Closed

Conversation

jcarvalho
Copy link

Which problem is this PR solving?

It upgrades to the latest version of Typescript.

Fixes #3955

Short description of the changes

Upgrades Typescript and Typedoc (as the version in use wasn't compatible with TS 5).

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

  • Typescript compilation works as expected
  • Unit tests run as expected

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@jcarvalho jcarvalho requested a review from a team November 25, 2023 21:19
Copy link

linux-foundation-easycla bot commented Nov 25, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@dyladan dyladan linked an issue Dec 1, 2023 that may be closed by this pull request
@dyladan
Copy link
Member

dyladan commented Dec 1, 2023

@jcarvalho seems the CI is not working. I seem to remember this is from kubecon contribfest right? Are you still working on this or should someone else pick it up?

@jcarvalho
Copy link
Author

This came from the Contribfest, yes, but haven't yet had the chance to look at the CI errors, will look into it during the weekend.

@jcarvalho
Copy link
Author

@dyladan The fix was actually quite simple (updated initially to TS 5.2, but then TS 5.3 came out and my cached typedoc dependency didn't bump into the dependency conflict).

package.json Show resolved Hide resolved
@dyladan
Copy link
Member

dyladan commented Dec 1, 2023

npm run lint:fix should fix your linting issues

Copy link

codecov bot commented Dec 1, 2023

Codecov Report

Merging #4323 (2f43ba0) into next (543f0b4) will decrease coverage by 1.92%.
Report is 87 commits behind head on next.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4323      +/-   ##
==========================================
- Coverage   92.24%   90.32%   -1.92%     
==========================================
  Files         332      161     -171     
  Lines        9437     3783    -5654     
  Branches     1999      841    -1158     
==========================================
- Hits         8705     3417    -5288     
+ Misses        732      366     -366     

see 176 files with indirect coverage changes

@jcarvalho
Copy link
Author

jcarvalho commented Dec 1, 2023

The EoL Node Tests failing are to be expected, as Typescript 5 requires Node 12, and the earliest version of Typedoc that supports TS 5.2 or higher requires Node 16 (although it seems the Node 14 tests are still passing?).

I seems that if we stick to Typescript 5.1 we should be able to stay in a version of Typedoc that still supports Node 12. What's the earliest version of Node you're targeting for the next release?

@dyladan
Copy link
Member

dyladan commented Dec 1, 2023

What's the earliest version of Node you're targeting for the next release?

We had hoped to bump to latest, but if it messes with API EOL testing we'll have to take a look. It might be possible to install old versions of typescript for just those tests.

@jcarvalho
Copy link
Author

Ah, looks like it's not related to Typedoc at all. Typescript 5.0 bumped the minimum Node version to 12, and 5.1 bumped it to 14 Link (which explains the cryptic error we're seeing now).

It might be possible to install old versions of typescript for just those tests.

Do you mean using this? If we do so, we might be limiting ourselves to not using Typescript features from 5.1 and above, and in that case it might just be better to stick to the Typescript 5.0 line (and require Node 12 or higher).

@dyladan
Copy link
Member

dyladan commented Dec 1, 2023

Ah, looks like it's not related to Typedoc at all. Typescript 5.0 bumped the minimum Node version to 12, and 5.1 bumped it to 14 Link (which explains the cryptic error we're seeing now).

It might be possible to install old versions of typescript for just those tests.

Do you mean using this? If we do so, we might be limiting ourselves to not using Typescript features from 5.1 and above, and in that case it might just be better to stick to the Typescript 5.0 line (and require Node 12 or higher).

Why would that limit the typescript features we can use? The EOL tests don't affect anything outside of the API module. Also, even later versions of typescript can still have their output run by old node versions, the compiler just doesn't run on those versions. A user with node 8 not using typescript could definitely use code emitted by the typescript 5.2 compiler.

@jcarvalho
Copy link
Author

Also, even later versions of typescript can still have their output run by old node versions, the compiler just doesn't run on those versions. A user with node 8 not using typescript could definitely use code emitted by the typescript 5.2 compiler.

Based on Typescript 5.1's Release Notes, it seems that Node 14.17 is a minimum Runtime Requirement, as that version of Typescript emits Javascript that requires Node 14 or higher, so even if we compiled on a newer version of Node, the Javascript output by the compilation process wouldn't run on the older Node versions.

@dyladan
Copy link
Member

dyladan commented Dec 13, 2023

Based on Typescript 5.1's Release Notes, it seems that Node 14.17 is a minimum Runtime Requirement, as that version of Typescript emits Javascript that requires Node 14 or higher, so even if we compiled on a newer version of Node, the Javascript output by the compilation process wouldn't run on the older Node versions.

Ah interesting. That should be OK everywhere except the API.

@jcarvalho
Copy link
Author

Given even 5.0 requires a Node 12 or higher at runtime, and we're still supporting Node 8 and 10, should we just skip the Typescript 5 upgrade for the API package (leaving it with TS 4), and upgrade everything else to the latest TS 5.3?

@Flarna
Copy link
Member

Flarna commented Dec 14, 2023

Based on Typescript 5.1's Release Notes, it seems that Node 14.17 is a minimum Runtime Requirement, as that version of Typescript emits Javascript that requires Node 14 or higher

I have a different understanding. This is the requirement to run typescript. The emitted code depends on the target config.

typescript is a dev dependency. So even if we generated code which runs on node 8 it doesn't require to run typescript using node 8. There is no need to build and run with the same version.

But typescript might emit .d.ts files which are not understood by older typescript versions. Therefore the requirement to build with recent versions leaks into user setups.

I don't think sticking on older typescript is a good choice even if it is only the API package. It's just a matter of time that we end up in compatibility issues because of this because some other tool in use expects more up to date peers.

@dyladan
Copy link
Member

dyladan commented Dec 15, 2023

Based on Typescript 5.1's Release Notes, it seems that Node 14.17 is a minimum Runtime Requirement, as that version of Typescript emits Javascript that requires Node 14 or higher

I have a different understanding. This is the requirement to run typescript. The emitted code depends on the target config.

typescript is a dev dependency. So even if we generated code which runs on node 8 it doesn't require to run typescript using node 8. There is no need to build and run with the same version.

This was also my understanding.

But typescript might emit .d.ts files which are not understood by older typescript versions. Therefore the requirement to build with recent versions leaks into user setups.
I don't think sticking on older typescript is a good choice even if it is only the API package. It's just a matter of time that we end up in compatibility issues because of this because some other tool in use expects more up to date peers.

Since typescript introduces these changes sometimes in minor releases, we can really only guarantee our output builds with the version we use (or later). Because this introduces a build-time node version dependency on our users, it is effectively a minimum node version. It may be possible to build with a later version and still run on the old node version but in practice most won't do this and I wouldn't want to encourage it. We're going to have to drop support for node 8 in the API eventually, and if we're not going to rev the major version, we're going to have to do it in a minor version at some point. Today may not be that day, but we should probably consider how we want to go about that change.

Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

@github-actions github-actions bot added the stale label Feb 26, 2024
Copy link

This PR was closed because it has been stale for 14 days with no activity.

@github-actions github-actions bot closed this Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[all] update dependency typescript to latest
3 participants