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 5.6 #9795

Merged
merged 10 commits into from
Sep 17, 2024
Merged

Typescript 5.6 #9795

merged 10 commits into from
Sep 17, 2024

Conversation

turadg
Copy link
Member

@turadg turadg commented Jul 29, 2024

evergreen

Description

https://devblogs.microsoft.com/typescript/announcing-typescript-5-6

Notable changes for this repo,

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

none

Testing Considerations

I captured type coverage before and after.

Upgrade Considerations

n/a

Copy link

cloudflare-workers-and-pages bot commented Jul 29, 2024

Deploying agoric-sdk with  Cloudflare Pages  Cloudflare Pages

Latest commit: 37e43c6
Status: ✅  Deploy successful!
Preview URL: https://9063a5b3.agoric-sdk.pages.dev
Branch Preview URL: https://typescript-5-6.agoric-sdk.pages.dev

View logs

Copy link
Member

@mhofman mhofman left a comment

Choose a reason for hiding this comment

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

My understanding is that 5.6 went final last night, so let's switch to that instead of the rc. It also seems that they may have reverted some changes that caused some of the Node and Iterator types compatibility issues that had to be worked around here, so let's possibly consider stashing those for now.

Comment on lines 273 to 407
[Symbol.asyncDispose]() {
throw Error('asyncDispose not implemented.');
},
Copy link
Member

Choose a reason for hiding this comment

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

According to the proposal, on async iterators this is the equivalent of calling return().

Suggested change
[Symbol.asyncDispose]() {
throw Error('asyncDispose not implemented.');
},
async [Symbol.asyncDispose]() {
await reader.return();
},

@mhofman
Copy link
Member

mhofman commented Sep 11, 2024

@turadg force-pushed the typescript-5.6 branch from e119569 to 7da7902 1 hour ago

As a matter of process, it's really difficult to review what changed from my previous review when the base commit this PR is staged on changed since the comparison of changes since last review is now broken. In general while the review is in progress, prefer merging changes from master if you must update the PR so that changes remain additive.

It also seems that they may have reverted some changes that caused some of the Node and Iterator types compatibility issues that had to be worked around here

Have we tried reverting the version bump of @types/node ? I am somewhat concerned about adding typing information for an API level we do not support.

@turadg
Copy link
Member Author

turadg commented Sep 11, 2024

As a matter of process…

I'll leave process discussions to internal forums.

Have we tried reverting the version bump of @types/node ? I am somewhat concerned about adding typing information for an API level we do not support.

Reverting the commit cause dozens of errors, all with the Buffer type due to IterableIterator types:

packages/internal/src/netstring.js:17:25 - error TS2322: Type 'Buffer' is not assignable to type 'Uint8Array'.
  The types returned by 'entries()' are incompatible between these types.
    Type 'IterableIterator<[number, number]>' is missing the following properties from type 'ArrayIterator<[number, number]>': map, filter, take, drop, and 9 more.

17   return Buffer.concat([prefix, data, suffix]);
                           ~~~~~~

We run CI tests with Node 18 so the only risk of Node 22 types is incorrect static analysis.

@mhofman
Copy link
Member

mhofman commented Sep 11, 2024

due to IterableIterator types

Ah got it, it's due to Iterator helpers addition, not the other iterable object type change.

The problem is most likely because we target esnext in our tsconfig, instead of being more conservative.

We run CI tests with Node 18 so the only risk of Node 22 types is incorrect static analysis.

Of course, my goal was to keep the tools working as best as possible, and the development iteration short.

@turadg turadg added the automerge:rebase Automatically rebase updates, then merge label Sep 11, 2024
@turadg turadg force-pushed the typescript-5.6 branch 4 times, most recently from dcf506f to 3f872ab Compare September 17, 2024 15:45
@turadg turadg added the bypass:integration Prevent integration tests from running on PR label Sep 17, 2024
@turadg
Copy link
Member Author

turadg commented Sep 17, 2024

Bypassed integration because this PR has passed integration several times save for multichain-testing, and I just reverted the changes to that.

@mergify mergify bot merged commit b5575d3 into master Sep 17, 2024
81 checks passed
@mergify mergify bot deleted the typescript-5.6 branch September 17, 2024 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge bypass:integration Prevent integration tests from running on PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants