more ts syntax #8335

turadg · 2023-09-14T23:06:44Z

Description

Motivation was to be able to document the promises in BootstrapSpace. That required adopting .ts syntax. I experimented with some more conversions.

Security Considerations

none

Scaling Considerations

n/a

Documentation Considerations

We should convert the whole codebase to this style for consistency,
-#8342

Testing Considerations

CI suffices

Upgrade Considerations

n/a

mhofman · 2023-09-14T23:09:38Z

packages/vats/src/core/types.ts

This should be types.d.ts no?

A '*.ts' file does seem to be unprecedented.

We have some *.d.ts and some *.test-d.ts (including one in vat-data/src) but no *.ts:

$ find . -name node_modules -prune -o -name moddable -prune -o \ -name '*.d.ts' -name '*.test-d.ts' -prune -o -prune -o \ -name '*.ts' -print $

To be clear, I have no problem with .ts files for tests, and those do exist in the endo repo, I guess not (yet) in agoric-sdk.

And it's very much possible that a bare .ts works in this case too, but I would find it too confusing and error prone as it's semantically different than types declarations only, which we want.

mhofman · 2023-09-14T23:11:12Z

Small reviewing nit: it would have been nice to have a commit renaming / converting type.js -> types.d.ts, then a second commit adding back the "empty" types.js for diffing purposes and possibly for conflict resolutions in the future.

dckc

I'm hesitant to make a runtime change (however small) to the bootstrap vat in a PR about changing how types are written.

dckc · 2023-09-15T15:54:29Z

packages/zoe/src/contractFacet/types.d.ts

+ * Informs Zoe about an issuer and returns a promise for acknowledging
+ * when the issuer is added and ready.
+ */
+type SaveIssuer = (


I suppose it pre-dates this PR, but...

This type suggests there might be other implementations of SaveIssuer. Seems like it should be inlined.

packages/vats/src/core/chain-behaviors.js

dckc · 2023-09-15T16:15:06Z

packages/zoe/test/offerArgsUsageContract.js

@@ -2,7 +2,9 @@
 const start = zcf => {
  /** @type {OfferHandler} */
  const handler = (_seat, offerArgs) => {
+    // @ts-expect-error xxx HandleOffer


I'm a bit reluctant to speak for @Chris-Hibbert and @erights re maintaining zoe package types in typescript rather than JSDoc, but I suppose this sort of thing shows that maintenance was suffering anyway.

This approach seems to get us closer to single-source for API docs (#4291). @erights said that made would make this well worth it.

mhofman · 2023-09-15T16:24:16Z

packages/vats/src/core/types.js

- */
-
-/** @template T @typedef {{vatPowers: { D: DProxy }, devices: T}} BootDevices<T> */
+// Dummy file for .ts twin to declare ambients


Ambient ts is an interesting approach...

packages/vats/src/core/chain-behaviors.js

Chris-Hibbert

just questions. I'm no authority.

Chris-Hibbert · 2023-09-15T17:34:54Z

packages/vats/src/core/types.ts

+        >
+      >;
+      walletFactory: Promise<
+        Installation<import('@agoric/smart-wallet/src/walletFactory.js').start>


Is this a typo, or is typeof required here? The cases above needed it.

Suggested change

Installation<import('@agoric/smart-wallet/src/walletFactory.js').start>

Installation<typeof import('@agoric/smart-wallet/src/walletFactory.js').start>

yes. .ts syntax is less forgiving that .js/JSDoc syntax in this regard

Chris-Hibbert · 2023-09-15T17:49:42Z

packages/vats/src/core/types.ts

+type AgoricNamesVat = ERef<
+  ReturnType<typeof import('../vat-agoricNames').buildRootObject>
+>;


Does typescript allow one to make shorthand for the oft-repeated ERef<ReturnType<typeof importedThing.buildRootObject>>?

Yes, good idea! We can have a utility type for the root object of a vat.

I tried DRYing out the buildRootObject reference but I haven't found a way to get the module import to be a generic type for s helper. It might require a module import, which would break this being an ambient.

We could DRY the ERef<ReturnType< but it wouldn't save a lot of characters unless we also do a helper like ContractInstallationPromises (which I just added). I expect eventually we'll want one for devices to be keys in a namespace instead of named by concatenation

erights · 2023-09-15T19:09:42Z

packages/vats/tsconfig.json

@@ -9,6 +9,7 @@
    "*.js",
    "scripts/**/*.js",
    "src/**/*.js",
+    "src/**/*.ts",


Should this be

Suggested change

"src/**/*.ts",

"src/**/*.d.ts",

What's here works, but would allow src to have runtime .ts files so I think your suggestion is worth including. I'll do it in a cleanup push

packages/zoe/src/contractFacet/types.js

dckc

I trust you'll also reach consensus with @mhofman and co.

mhofman

Ok I think my confusion came from the fact that some bundlers or other pieces of tooling would fail if importing a js file that isn't a module by heuristics (aka doesn't have any import or export statement), but apparently that's not a problem since both before and after the type.js file doesn't have those statements, and it is runtime imported. So all clear on my end, sorry for the confusion.

turadg · 2023-09-16T05:11:02Z

came from the fact that some bundlers or other pieces of tooling would fail if importing a js file that isn't a module by heuristics (aka doesn't have any import or export statement)

Your concern was well-placed! The typescript-eslint plugin was failing to load the empty .js files. I have included this work-around: 365be75

turadg · 2023-09-17T16:51:34Z

The eslintconfig change wasn't reviewed. I merged to reduce the risk of merge conflicts and ease building atop it. But if anyone has a better solution or request I'm happy to hear them and will follow up.

turadg requested review from mhofman and dckc September 14, 2023 23:06

mhofman reviewed Sep 14, 2023

View reviewed changes

mhofman self-requested a review September 14, 2023 23:12

dckc reviewed Sep 15, 2023

View reviewed changes

mhofman reviewed Sep 15, 2023

View reviewed changes

Chris-Hibbert reviewed Sep 15, 2023

View reviewed changes

erights reviewed Sep 15, 2023

View reviewed changes

packages/zoe/src/contractFacet/types.js Outdated Show resolved Hide resolved

dckc approved these changes Sep 15, 2023

View reviewed changes

turadg requested a review from mhofman September 16, 2023 03:51

mhofman approved these changes Sep 16, 2023

View reviewed changes

turadg force-pushed the ta/doc-bootspace-powers branch from cd7c7ee to 365be75 Compare September 16, 2023 05:09

turadg mentioned this pull request Sep 16, 2023

convert all type declaration files to TS syntax (.d.ts) #8342

Open

turadg force-pushed the ta/doc-bootspace-powers branch 2 times, most recently from 6aea57b to c6c5acb Compare September 16, 2023 14:58

turadg added the automerge:rebase Automatically rebase updates, then merge label Sep 16, 2023

turadg added 10 commits September 16, 2023 20:49

chore(types): convert core/types.j to .ts

b87925c

chore(deps): explicit import NameHub

6ce0d2c

docs(types): describe some bootspace promises

814ea6e

chore(types): convert src/types from .js to .ts

cbde2ea

chore(types): convert contractFacet/types to .ts

4e00f32

chore(types): suppression updates

804e7a3

refactor(types): inline some ZCF methods

be96a22

refactor(types): ContractInstallationPromises helper

027f4a1

refactor(types): use types-ambient naming

580d20c

ci(types): skip TS for types-ambient

7edc3e9

turadg force-pushed the ta/doc-bootspace-powers branch from c6c5acb to 7edc3e9 Compare September 16, 2023 20:49

mergify bot merged commit 3da32ff into master Sep 16, 2023
71 checks passed

mergify bot deleted the ta/doc-bootspace-powers branch September 16, 2023 21:29

turadg mentioned this pull request Sep 18, 2023

adopt TypeScript .ts syntax #5760

Open

dckc mentioned this pull request Nov 1, 2023

types for permissioned contract deployment not exported #8328

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more ts syntax #8335

more ts syntax #8335

turadg commented Sep 14, 2023 •

edited

Loading

mhofman Sep 14, 2023

dckc Sep 15, 2023

mhofman Sep 15, 2023

mhofman commented Sep 14, 2023 •

edited

Loading

dckc left a comment

dckc Sep 15, 2023

dckc Sep 15, 2023

dckc Sep 15, 2023

mhofman Sep 15, 2023

Chris-Hibbert left a comment

Chris-Hibbert Sep 15, 2023

turadg Sep 15, 2023

Chris-Hibbert Sep 15, 2023

turadg Sep 15, 2023

turadg Sep 15, 2023

erights Sep 15, 2023

turadg Sep 16, 2023

dckc left a comment

mhofman left a comment

turadg commented Sep 16, 2023 •

edited

Loading

turadg commented Sep 17, 2023

	Installation<import('@agoric/smart-wallet/src/walletFactory.js').start>
	Installation<typeof import('@agoric/smart-wallet/src/walletFactory.js').start>

more ts syntax #8335

more ts syntax #8335

Conversation

turadg commented Sep 14, 2023 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofman commented Sep 14, 2023 • edited Loading

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

Chris-Hibbert 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

Choose a reason for hiding this comment

dckc left a comment

Choose a reason for hiding this comment

mhofman left a comment

Choose a reason for hiding this comment

turadg commented Sep 16, 2023 • edited Loading

turadg commented Sep 17, 2023

turadg commented Sep 14, 2023 •

edited

Loading

mhofman commented Sep 14, 2023 •

edited

Loading

turadg commented Sep 16, 2023 •

edited

Loading