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): update typescript to 4.9.5 #3952

Closed
wants to merge 7 commits into from

Conversation

llc1123
Copy link
Contributor

@llc1123 llc1123 commented Jun 26, 2023

Which problem is this PR solving?

  • typedoc-plugin-resolve-crossmodule-references is now deprecated and typedoc >= 0.24.0 has the function built-in.
  • typedoc >= 0.24.0 requires typescript >= 4.6.0

Short description of the changes

  • Upgraded typescript to 4.9.5.
  • Fixed all conflict dependencies.
  • Fixed type errors.

** I tried upgrading to typescript@5 but it doesn't support node < 12 anymore so I used the last version ^4

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Existing Tests

Checklist:

  • Followed the style guidelines of this project

@llc1123 llc1123 requested a review from a team June 26, 2023 08:47
@codecov
Copy link

codecov bot commented Jun 26, 2023

Codecov Report

Merging #3952 (3e77bed) into main (070b685) will increase coverage by 0.01%.
The diff coverage is n/a.

❗ Current head 3e77bed differs from pull request most recent head 9ed9ad8. Consider uploading reports for the commit 9ed9ad8 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3952      +/-   ##
==========================================
+ Coverage   93.13%   93.14%   +0.01%     
==========================================
  Files         298      298              
  Lines        8869     8869              
  Branches     1826     1826              
==========================================
+ Hits         8260     8261       +1     
+ Misses        609      608       -1     

see 1 file with indirect coverage changes

@llc1123 llc1123 changed the title chore(deps): update typescript to 5.1.3 chore(deps): update typescript to 4.9.5 Jun 26, 2023
@llc1123 llc1123 force-pushed the chore/update-typescript branch from 72fac96 to 50cd98a Compare June 26, 2023 10:05
@Flarna
Copy link
Member

Flarna commented Jun 26, 2023

Is there a difference in the generated d.ts files which requires consumers to use also a newer TS version?
If yes I would expect such a change is breaking.

@llc1123
Copy link
Contributor Author

llc1123 commented Jun 26, 2023

Is there a difference in the generated d.ts files which requires consumers to use also a newer TS version?
If yes I would expect such a change is breaking.

I think it is safe upgrading minor versions.

@pichlermarc
Copy link
Member

pichlermarc commented Jun 26, 2023

I think we should leave this PR open a bit to allow people to review it. I'll also review the generated files to ensure we're not missing anything.

There can be breaking changes in typescript minor versions as typescript does not follow semver.

We'll have to look though:

https://devblogs.microsoft.com/typescript/announcing-typescript-4-5/#breaking-changes
https://devblogs.microsoft.com/typescript/announcing-typescript-4-6/#breaking-changes
https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#breaking-changes
https://devblogs.microsoft.com/typescript/announcing-typescript-4-8/#breaking-changes
https://devblogs.microsoft.com/typescript/announcing-typescript-4-9/#breaking-changes

and ensure we're not affected.

@llc1123
Copy link
Contributor Author

llc1123 commented Jun 26, 2023

I compared the output. All types in new d.ts files don't have the declare keyword. I don't see any other differences.

Before

export declare type HrTime = [number, number];

After

export type HrTime = [number, number];

I think it is safe to upgrade.

@pichlermarc
Copy link
Member

pichlermarc commented Jun 26, 2023

I compared the output. All types in new d.ts files don't have the declare keyword. I don't see any other differences.

Before

export declare type HrTime = [number, number];

After

export type HrTime = [number, number];

I think it is safe to upgrade.

Yes, the only other difference I've seen is

-export declare const parseResponseStatus: (kind: SpanKind, statusCode?: number | undefined) => SpanStatusCode;
+export declare const parseResponseStatus: (kind: SpanKind, statusCode?: number) => SpanStatusCode;

and that should also be fine from what I can tell.

Anyway: I'd rather be cautious so let's leave this PR open and see if others have a different opinion. If not then I'll merge this on Wednesday. 🙂

EDIT: A diff of the d.ts files for other reviewers to look at -> pichlermarc/inspect-3952@b4a2f4b

@pichlermarc pichlermarc added the needs-more-reviewers PRs with this label are ready for review and needs more people to review to move forward. label Jun 26, 2023
@dyladan
Copy link
Member

dyladan commented Jun 28, 2023

Is there a difference in the generated d.ts files which requires consumers to use also a newer TS version?
If yes I would expect such a change is breaking.

Is there a difference in the generated d.ts files which requires consumers to use also a newer TS version?
If yes I would expect such a change is breaking.

I think it is safe upgrading minor versions.

Marc is definitely correct there are breaking changes in minor TS versions and breaking changes in the output. There are projects like https://github.com/sandersn/downlevel-dts which allow you to publish types compatible with old ts versions in parallel with modern types.

The 2 changes to the output relevant to us are:

Looking at marc's diff I don't see anywhere these are used in the output, but they could be at any point in the future. Right now if we try to use these features Typescript will simply fail to compile. If we upgrade our TS version and some PR uses either of these features in the future then we will be breaking users.

Ideally I would like to see at least one of the following if not both in order to accept this PR:

  1. Use a project like downlevel-dts to publish old-ts-safe types alongside modern types
  2. A test which compiles a project using the API with typescript 4.4

@llc1123
Copy link
Contributor Author

llc1123 commented Jun 29, 2023

If we upgrade our TS version and some PR uses either of these features in the future then we will be breaking users.

Do we have such statement in the docs saying we support [email protected]? If not, I think doing such compatibility support is kind of redundant as it works for now and we don't promise to support [email protected] in the future.

BTW, I think we should upgrade to ts@5 in the next major release.

@pichlermarc
Copy link
Member

pichlermarc commented Jun 29, 2023

If we upgrade our TS version and some PR uses either of these features in the future then we will be breaking users.

Do we have such statement in the docs saying we support [email protected]? If not, I think doing such compatibility support is kind of redundant as it works for now and we don't promise to support [email protected] in the future.

We do this implicitly by adhering to semver.

BTW, I think we should upgrade to ts@5 in the next major release.

Yes, we discussed this a few times in the SIG meeting already, but we did not have an issue as of now. I created one and added it to the 2.0 milstone: #3955 🙂

@Flarna
Copy link
Member

Flarna commented Jun 29, 2023

API compatibility with different ts versions is hard in general. New ts versions may have better type checking which may result in broken builds. Also built in types in typescript may have impact.
Refs: #3636, #3660

Also minor changes in types only may cause breakage independent of the ts version. So keeping 100% compatibility with typescript in mind is quite hard, if only the pure JS API surface is taken it's by far easier.

We had several issues in the past that adding a private member in an exported class caused issues.

So maybe we have to even specify a fix ts version to ensure no breakage happens. But that would be quite bad.

@llc1123
Copy link
Contributor Author

llc1123 commented Jun 29, 2023

So maybe we have to even specify a fix ts version to ensure no breakage happens. But that would be quite bad.

In my opinion, users usually use the latest tsc in their root projects and using dts generated by old tsc also causes problems if the root version is newer just like #3636.

I suggest using tsc version as new as possible if it doesn't affect JS compatibilities.

@github-actions
Copy link

github-actions bot commented Sep 4, 2023

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 Sep 4, 2023
@pichlermarc
Copy link
Member

pichlermarc commented Sep 4, 2023

@llc1123, there has been some movement on this issue:

We plan to update typescript to the most recent version with a 2.0 release of the SDK (#4083), as we've concluded this will be a breaking change.

We may also introduce a policy that will allow us to bump typescript more regularly without having to bump the major version, similar to what other projects do, though this is still up for debate (i'll link the issue here once it's available). Though with the currently proposed approach of one major release each year, we'll probably also be able to keep typescript reasonably up-to-date without this policy.

Edit: note that SDK 2.0 is still in a planning phase, actual implementation work will start in October (current estimation).

@llc1123
Copy link
Contributor Author

llc1123 commented Sep 4, 2023

LGTM. I've been really busy recently and didn't have time to look at the updates. I'll be back in October and take care of the remaining issues.

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 Nov 20, 2023
@pichlermarc
Copy link
Member

There's currently a PR incoming from #3955 which will bump typescript to 5.2 (this is for the 2.0 release)
We'll not go with the route of allowing typescript bumps in minor versions, but we'll instead:

  • bump to the latest typescript version available on SDK 2.0 release
  • do regular major bumps to otel js (every one to two years) that'll allow us to keep everything reasonably up-to-date.

@pichlermarc
Copy link
Member

I'll close this PR in the meantime.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-more-reviewers PRs with this label are ready for review and needs more people to review to move forward. stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants