TypeScript 5.7 and fix coverage CI #10561

turadg · 2024-11-24T01:01:05Z

evergreen

Description

Bump Typescript to the 5.7 release
https://devblogs.microsoft.com/typescript/announcing-typescript-5-7/

This also corrects a typo in the coverage job from #10560

Security Considerations

none

Scaling Considerations

none

Documentation Considerations

none

Testing Considerations

CI

Upgrade Considerations

none

cloudflare-workers-and-pages · 2024-11-24T01:01:55Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`140ff5f`
Status:	✅ Deploy successful!
Preview URL:	https://62e8cd0f.agoric-sdk.pages.dev
Branch Preview URL:	https://typescript-5-7.agoric-sdk.pages.dev

View logs

michaelfig · 2024-11-24T02:32:15Z

.github/workflows/after-merge.yml

@@ -106,7 +106,7 @@ jobs:
          node-version: ${{ matrix.node-version }}

      - name: generate test coverage report
-        run: ./scripts/ci/generage-test-coverage-report.sh


Yeah, I should have noticed this one. 😛

michaelfig · 2024-11-24T03:00:52Z

yarn.lock

I'm a little leery of advancing to typescript@~5.7.1 when we're still getting errors that look like:

WARNING: You are currently running a version of TypeScript which is not officially supported by @typescript-eslint/typescript-estree. You may find that it works just fine, or you may not. SUPPORTED TYPESCRIPT VERSIONS: >=4.7.4 <5.6.0 YOUR TYPESCRIPT VERSION: 5.6.3 Please only submit bug reports when using the officially supported version.

Even if we upgraded to typescript-eslint@^8.10.0 to get support for typescript@^5.6.0 (as https://github.com/typescript-eslint/typescript-eslint/releases/tag/v8.10.0 describes), there isn't yet a version of typescript-eslint that claims to support [email protected].

Can we slow down our upgrades of typescript a little bit and first focus on upgrading to a version of the typescript-eslint toolchain that is officially supported?

Fair. I've been working on that but it hasn't been a priority:

chore(deps): bump typescript-eslint to 8.14 #10120

The reason I need TS 5.7 is for #10480 . Without it,

Error: tsconfig.build.json(7,5): error TS5096: Option 'allowImportingTsExtensions' can only be used when either 'noEmit' or 'emitDeclarationOnly' is set.

The status quo is that we have this warning from typescript-eslint. Landing this PR doesn't change that. By the time we do update typescript-eslint, it will support 5.7 (already in master).

So I ask that you allow this PR to merge and if you want the lint error to go away you lobby that it be prioritized (vs other work, ofc).

I am not too concerned with the eslint warnings. They're par for the course

By the time we do update typescript-eslint, it will support 5.7 (typescript-eslint/typescript-eslint#10372).

I see [email protected] support is already merged in typescript-eslint master, is currently in a canary release and will be officially released. That quells my concerns.

So I ask that you allow this PR to merge

Thanks for the additional context. Approved.

mhofman

LGTM, except for a question on the runtime change in the reserve terms.

mhofman · 2024-11-24T06:47:46Z

packages/inter-protocol/src/provisionPoolKit.js

@@ -37,6 +37,15 @@ const FIRST_LOWER_NEAR_KEYWORD = /^[a-z][a-zA-Z0-9_$]*$/;
 * @import {Bank, BankManager} from '@agoric/vats/src/vat-bank.js'
 */

+// XXX when inferred, error TS2742: cannot be named without a reference to '../../../node_modules/@endo/exo/src/get-interface.js'. This is likely not portable. A type annotation is necessary.


I don't understand this error but explicit return values is totally acceptable anyway

mhofman · 2024-11-24T06:54:39Z

packages/inter-protocol/src/reserve/params.js

@@ -2,8 +2,10 @@

 import { CONTRACT_ELECTORATE, ParamTypes } from '@agoric/governance';

-const makeReserveTerms = (poserInvitationAmount, timer) => ({
-  timer,


This is a change to the runtime behavior. A new chain will now no longer have the timer in the reserve terms where before that it would have. What is the motivation?

Motivation is the caller never provided the argument and it's now detected. The runtime impact is that property will be omitted from the return instead of being undefined. I don't see anything checking for the key presence specifically.

Given lack of value at the caller site, that change sounds reasonable

mhofman · 2024-11-24T06:57:37Z

yarn.lock

I am not too concerned with the eslint warnings. They're par for the course

mhofman · 2024-11-24T07:00:08Z

packages/xsnap/src/avaXS.js

@@ -112,7 +112,7 @@ async function runTestScript(
  /**
   * Handle callback "command" from xsnap subprocess.
   *
-   * @type { (msg: ArrayBuffer) => Promise<ArrayBuffer> }
+   * @type { (msg: ArrayBuffer) => Promise<Uint8Array<ArrayBufferLike>> }


This is a significant change of type. New one looks right but I'm surprised we didn't have an error before.

mhofman

Thanks for clarifying the reserve terms change

mhofman · 2024-11-24T18:15:10Z

packages/inter-protocol/src/reserve/params.js

@@ -2,8 +2,10 @@

 import { CONTRACT_ELECTORATE, ParamTypes } from '@agoric/governance';

-const makeReserveTerms = (poserInvitationAmount, timer) => ({
-  timer,


Given lack of value at the caller site, that change sounds reasonable

doesn't actually take or return a timer

turadg added 2 commits November 23, 2024 16:51

ci: fix 'coverage' job

42bb239

chore(deps): bump typescript to 5.7 release

e63b369

turadg requested review from michaelfig and 0xpatrickdev November 24, 2024 01:01

turadg requested a review from a team as a code owner November 24, 2024 01:01

turadg mentioned this pull request Nov 24, 2024

convert fast-usdc contract to .ts #10480

Draft

michaelfig reviewed Nov 24, 2024

View reviewed changes

turadg requested a review from michaelfig November 24, 2024 05:23

mhofman reviewed Nov 24, 2024

View reviewed changes

mhofman approved these changes Nov 24, 2024

View reviewed changes

turadg added the automerge:rebase Automatically rebase updates, then merge label Nov 24, 2024

turadg added 4 commits November 24, 2024 19:36

chore(types): fix Buffer usage in TS 5.7

292d2f3

chore(deps): yarn-deduplicate

9c28c53

fix: makeReserveTerms

27ce0b0

doesn't actually take or return a timer

chore(types): hack makeProvisionPoolKit

140ff5f

turadg force-pushed the typescript-5.7 branch from c6245e1 to 140ff5f Compare November 24, 2024 19:36

michaelfig approved these changes Nov 24, 2024

View reviewed changes

mergify bot merged commit 5b09631 into master Nov 24, 2024
81 checks passed

mergify bot deleted the typescript-5.7 branch November 24, 2024 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TypeScript 5.7 and fix coverage CI #10561

TypeScript 5.7 and fix coverage CI #10561

turadg commented Nov 24, 2024

cloudflare-workers-and-pages bot commented Nov 24, 2024 •

edited

Loading

michaelfig Nov 24, 2024

michaelfig Nov 24, 2024

turadg Nov 24, 2024 •

edited

Loading

mhofman Nov 24, 2024

michaelfig Nov 24, 2024

mhofman left a comment

mhofman Nov 24, 2024

mhofman Nov 24, 2024

turadg Nov 24, 2024

mhofman Nov 24, 2024

mhofman Nov 24, 2024

mhofman Nov 24, 2024

mhofman left a comment

mhofman Nov 24, 2024

TypeScript 5.7 and fix coverage CI #10561

TypeScript 5.7 and fix coverage CI #10561

Conversation

turadg commented Nov 24, 2024

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Nov 24, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Nov 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented Nov 24, 2024 •

edited

Loading

turadg Nov 24, 2024 •

edited

Loading