clean up types modules #9364

turadg · 2024-05-14T00:31:09Z

no ticket

Description

Started as documentation for #9356 (comment) , then I carried out the new best practice so it would be consistent. That was a bit of a rabbit hole.

It's in a better place, though I don't see it reflected obviously in the type coverage report.

Security Considerations

n/a, types

Scaling Considerations

n/a, types

Documentation Considerations

This documents the new beset practice

Testing Considerations

CI

Upgrade Considerations

n/a, types

packages/zoe/src/zoeService/utils.ts

dckc

Before I approve,

I'd like to hear from @Chris-Hibbert about the Timestamp vs. TimestampRecord change
I think I should understand the use of CapData in smart-wallet better

packages/smart-wallet/src/types.ts

dckc · 2024-05-14T16:07:40Z

packages/inter-protocol/src/auction/scheduleMath.js

- * @param {Timestamp} baseTime
+ * @param {TimestampRecord} baseTime
 * @returns {import('./scheduler.js').Schedule}
 */
 export const computeRoundTiming = (params, baseTime) => {


The test that's changed in eb64643 seems to show that the wider Timestamp type does work here. While narrowing the type is a longer-term goal, the fact that a test had to be changed shows that it's a breaking change.

@Chris-Hibbert what do you think about narrowing this type at this point?

I think this is movement forward. This particular change won't have wider impacts beyond the test, so it's fine to require compliance with the target state.

dckc · 2024-05-14T16:11:51Z

packages/swingset-liveslots/src/vatDataTypes.ts

  defineKind: <P, S, F>(
    tag: string,
+    // @ts-expect-error rest parameter must be of an array type
    init: (...args: P) => S,


must be array or Parameters type, right? I suppose there's no way to say "<P extends Parameters, ...>"?

I'm a little surprised that this works; i.e. that the types of the init parameters get checked. Do we happen to have a type test?

I didn't invest in fixing these because they're all on @deprecated types

packages/zoe/src/zoeService/utils.ts

turadg · 2024-05-14T17:24:31Z

packages/time/src/types.ts

@@ -290,17 +290,19 @@ export type TimeMathType = {
  ) => RelativeTimeRecord;
  /**
   * An absolute time + a relative time gives a new absolute time.
-   *
-   * @template {Timestamp} T


this was ignored before because it wasn't a .js file.

When I made converted the syntax, it narrowed too much all the way to the value literal. That's why made the conditional to return one of two types

turadg · 2024-05-14T19:42:07Z

gah. this will need more work.

Some packages export types at the top level like so: https://github.com/Agoric/agoric-sdk/blob/9553675cd136d389c427e024fc4da78558344779/packages/time/index.js#L3-L5

When those packages get bundled, the bundler can't find the file. This is good for safety of the bundler (it doesn't know anything about .ts) but it means we can't have .ts and export that way.

I'll see about exporting through the package.json exports patterns. But that will take a while so I'll drop this PR into draft.

Chris-Hibbert

looks fine to me.

Chris-Hibbert · 2024-05-14T21:39:48Z

packages/inter-protocol/src/auction/scheduleMath.js

- * @param {Timestamp} baseTime
+ * @param {TimestampRecord} baseTime
 * @returns {import('./scheduler.js').Schedule}
 */
 export const computeRoundTiming = (params, baseTime) => {


I think this is movement forward. This particular change won't have wider impacts beyond the test, so it's fine to require compliance with the target state.

cloudflare-workers-and-pages · 2024-05-15T19:52:19Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`c6c0a39`
Status:	✅ Deploy successful!
Preview URL:	https://9b5ea2d8.agoric-sdk.pages.dev
Branch Preview URL:	https://ta-use--ts.agoric-sdk.pages.dev

View logs

dckc · 2024-05-15T21:59:21Z

no ticket
...

Upgrade Considerations

n/a, types

Until we release from master (#9252), even types changes can have an impact on the cost of preparing a release.

I don't think this PR makes the job much harder than it already is, but I do keep that in mind.

cc @gibson042 @mhofman

dckc

I think I'll need get more experience with this to grok fully.

but for now, it looks like a step forward

to fix build order sensitivity

Helps avoid exporting private type

mhofman

These look like great improvements and documentation. My main concern is how this might indeed make cherry-picking more complex for PRs stacked on top of this.

mhofman · 2024-05-16T00:36:12Z

docs/typescript.md

+
+We have `"skipLibCheck": true"` in the root tsconfig.json because some libraries we depend on have their own type errors. (A massive one is the output of Telescope, used in `@agoric/cosmic-proto`.)
+
+This means that the types you write in `.d.ts` file won't be checked by `tsc`. To gain some confidence, you can temporarily flip that setting in a package's own `tsconfig.json` and pay attention to only the relevant errors.


I didn't realize that, yikes!

mhofman · 2024-05-16T00:37:54Z

docs/typescript.md

 - package entrypoint(s) exports explicit types
 - for packages upon which other packages expect ambient types:
  - `exported.js` exports the explicit types and ambient re-exports

+## .d.ts modules
+
+We cannot use `.ts` files any modules that are transitively imported into an Endo bundle. The reason is that the Endo bundler doesn't understand `.ts` syntax and we don't want it to until we have sufficient auditability of the transformation. Moreover we've tried to avoid a build step in order to import a module. (The one exception so far is `@agoric/cosmic-proto` because we codegen the types. Those modules are written in `.ts` syntax and build to `.js` by a build step that creates `dist`, which is the package export.)


Is the dist not checked in? I didn't realize we introduced a required build step. At least those files don't really change much.

mhofman · 2024-05-16T00:40:35Z

docs/typescript.md

+
+One option considered is having the conditional package `"exports"` include `"types"` but that has to be a .d.ts file. That could be generated from a `.ts` but it would require a build step, which we've so far avoided.
+
+Once we have [JSDoc export type support](https://github.com/microsoft/TypeScript/issues/48104) we'll be able instead to keep the `index.js` entrypoint and have it export the types from `.ts` files without a runtime import of the module containing them.


That would be great indeed. I would likely still want the .ts file to only contain declare statements.

turadg · 2024-05-16T17:07:41Z

These look like great improvements and documentation.
Thanks.

My main concern is how this might indeed make cherry-picking more complex for PRs stacked on top of this.

To help I've reduced the number of changes. I hope they'll be obvious to resolve in a merge.

Another reason I reduced them is I realized the pack check should really be a prepack check (documented in the CI script) and some of the cleanup wasn't compatible with that.

refs: #9364 ## Description Follow-up to #9364, also doing the `skipLibCheck: false` for Zoe package and fixing up most of the issues. (Documenting the deferred). ### Security Considerations  ### Scaling Considerations  ### Documentation Considerations  ### Testing Considerations  ### Upgrade Considerations just types and tests. will not affect production but small impact on release process (cherry picking)

turadg requested review from dckc and 0xpatrickdev May 14, 2024 00:31

turadg commented May 14, 2024

View reviewed changes

packages/zoe/src/zoeService/utils.ts Outdated Show resolved Hide resolved

turadg mentioned this pull request May 14, 2024

Orch api factoring #9356

Merged

dckc reviewed May 14, 2024

View reviewed changes

turadg commented May 14, 2024

View reviewed changes

turadg marked this pull request as draft May 14, 2024 19:42

Chris-Hibbert approved these changes May 14, 2024

View reviewed changes

turadg force-pushed the ta/use-.ts branch from 93487ac to 65f1cb3 Compare May 15, 2024 19:52

turadg marked this pull request as ready for review May 15, 2024 20:11

turadg requested a review from dckc May 15, 2024 20:11

chore(deps): clean up unused

97c57c7

turadg force-pushed the ta/use-.ts branch from 65f1cb3 to 981a46c Compare May 15, 2024 21:51

dckc approved these changes May 15, 2024

View reviewed changes

turadg force-pushed the ta/use-.ts branch from 5d815cb to 259b6c5 Compare May 15, 2024 22:23

turadg added 7 commits May 15, 2024 16:50

fix(types): include .cjs

416818b

test: conform to time type

b02c004

chore: rm empty types module

0985eeb

chore: update type-coverage

41460f5

chore(types): more explicit imports

5140008

to fix build order sensitivity

chore(types): simplify StartFunction tagging

85fb5ac

Helps avoid exporting private type

refactor(types): @import

caad41f

turadg force-pushed the ta/use-.ts branch from 259b6c5 to a925bba Compare May 16, 2024 00:02

mhofman reviewed May 16, 2024

View reviewed changes

turadg added the automerge:rebase Automatically rebase updates, then merge label May 16, 2024

turadg changed the title ~~use .ts for types modules~~ clean up types modules May 16, 2024

turadg removed the automerge:rebase Automatically rebase updates, then merge label May 16, 2024

turadg added 3 commits May 16, 2024 09:31

docs: use of .d.ts files

b269707

fix(types): problems hidden by skipLibCheck

6a6e595

chore: update type-coverage

d371cb3

turadg force-pushed the ta/use-.ts branch from a925bba to d371cb3 Compare May 16, 2024 16:31

turadg added 4 commits May 16, 2024 09:34

ci: test prepack ordering

73a1c11

chore: export modules map for its type export

f39d40f

style: consistent spacing of triple-slash

b2306a8

refactor(types): references for ambients

c6c0a39

turadg added the automerge:rebase Automatically rebase updates, then merge label May 16, 2024

mergify bot merged commit 0f94e34 into master May 16, 2024
71 checks passed

mergify bot deleted the ta/use-.ts branch May 16, 2024 18:18

turadg mentioned this pull request May 16, 2024

Zoe .d.ts libcheck #9374

Merged

This was referenced May 26, 2024

dependency-graph causes restore-node cache hits to fail #9412

Closed

WIP: adapt a3p-integration for post-upgrade-15 Docker images #9414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean up types modules #9364

clean up types modules #9364

turadg commented May 14, 2024

dckc left a comment

dckc May 14, 2024

Chris-Hibbert May 14, 2024

dckc May 14, 2024

turadg May 14, 2024

turadg May 14, 2024

turadg commented May 14, 2024

Chris-Hibbert left a comment

Chris-Hibbert May 14, 2024

cloudflare-workers-and-pages bot commented May 15, 2024 •

edited

Loading

dckc commented May 15, 2024

Upgrade Considerations

dckc left a comment

mhofman left a comment

mhofman May 16, 2024

mhofman May 16, 2024

turadg May 16, 2024

mhofman May 16, 2024

turadg May 16, 2024

turadg commented May 16, 2024


		We have `"skipLibCheck": true"` in the root tsconfig.json because some libraries we depend on have their own type errors. (A massive one is the output of Telescope, used in `@agoric/cosmic-proto`.)

		This means that the types you write in `.d.ts` file won't be checked by `tsc`. To gain some confidence, you can temporarily flip that setting in a package's own `tsconfig.json` and pay attention to only the relevant errors.


		One option considered is having the conditional package `"exports"` include `"types"` but that has to be a .d.ts file. That could be generated from a `.ts` but it would require a build step, which we've so far avoided.

		Once we have [JSDoc export type support](https://github.com/microsoft/TypeScript/issues/48104) we'll be able instead to keep the `index.js` entrypoint and have it export the types from `.ts` files without a runtime import of the module containing them.

clean up types modules #9364

clean up types modules #9364

Conversation

turadg commented May 14, 2024

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

dckc 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

turadg commented May 14, 2024

Chris-Hibbert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cloudflare-workers-and-pages bot commented May 15, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

dckc commented May 15, 2024

Upgrade Considerations

dckc left a 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

turadg commented May 16, 2024

cloudflare-workers-and-pages bot commented May 15, 2024 •

edited

Loading