opentelemetry-js icon indicating copy to clipboard operation
opentelemetry-js copied to clipboard

chore(deps): Upgrade to Typescript 5

Open jcarvalho opened this issue 2 years ago • 16 comments

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

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

How Has This Been Tested?

  • [x] Typescript compilation works as expected
  • [x] Unit tests run as expected

Checklist:

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

jcarvalho avatar Nov 25 '23 21:11 jcarvalho

CLA Signed

The committers listed above are authorized under a signed CLA.

  • :white_check_mark: login: jcarvalho / name: João Carvalho (4c7a3f615784e895f0a5923b9a6804121f07de20, 4a681e8f5d24a245d999b93438163265ad6900da, 2f43ba09a20e03f1fef0fd61d196839d7c221d91)

@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?

dyladan avatar Dec 01 '23 20:12 dyladan

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 avatar Dec 01 '23 20:12 jcarvalho

@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).

jcarvalho avatar Dec 01 '23 20:12 jcarvalho

npm run lint:fix should fix your linting issues

dyladan avatar Dec 01 '23 20:12 dyladan

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

codecov[bot] avatar Dec 01 '23 20:12 codecov[bot]

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?

jcarvalho avatar Dec 01 '23 20:12 jcarvalho

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.

dyladan avatar Dec 01 '23 20:12 dyladan

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).

jcarvalho avatar Dec 01 '23 21:12 jcarvalho

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.

dyladan avatar Dec 01 '23 21:12 dyladan

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.

jcarvalho avatar Dec 01 '23 21:12 jcarvalho

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.

dyladan avatar Dec 13 '23 16:12 dyladan

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?

jcarvalho avatar Dec 14 '23 00:12 jcarvalho

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.

Flarna avatar Dec 14 '23 07:12 Flarna

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.

dyladan avatar Dec 15 '23 17:12 dyladan

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[bot] avatar Feb 26 '24 06:02 github-actions[bot]

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

github-actions[bot] avatar Mar 25 '24 06:03 github-actions[bot]