clean up types modules (#9364)

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
Agoric · May 16, 2024 · 0f94e34 · 0f94e34
2 parents c56324b + c6c0a39
commit 0f94e34
Show file tree

Hide file tree

Showing 70 changed files with 187 additions and 144 deletions.
diff --git a/.github/workflows/test-all-packages.yml b/.github/workflows/test-all-packages.yml
@@ -62,8 +62,11 @@ jobs:
       # attempting to merge to master. This takes about 1min locally and since
       # this job is about 30s in CI doing it here doesn't add to wall wait
       # for CI resolution.
+      # We prepack only because full pack removes the declarations, relying on ambient
+      # resolution in the repo filesystem which will not be available when the packages
+      # are pulled from npm.
       - name: Pack packages
-        run: yarn lerna exec --reject-cycles --concurrency 1 "npm pack"
+        run: yarn lerna run --reject-cycles --concurrency 1 prepack
 
   ##################
   # Lint tests

diff --git a/docs/typescript.md b/docs/typescript.md
@@ -4,18 +4,60 @@ Our use of TypeScript has to accomodate both .js development in agoric-sdk (whic
 
 ## Best practices
 
+- `.d.ts` for types modules
 - 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.)
+
+A `.d.ts` module allows defining the type in `.ts` syntax, without any risk that it will be included in runtime code. The `.js` is what actually gets imported.
+
+The are some consequences to this approach.
+
+### File pair
+
+You have to create a `.js` and `.d.ts` pair for each module. Usually it's of the form,
+
+```js
+// Empty JS file to correspond with its .d.ts twin
+export {};
+```
+
+### Lack of type checking
+
+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.
+
+### Alternatives
+
+We've experimented with having `.ts` files. It works, and gets around the skipLibCheck problem, but it complicates the build and exports. It also necessitates a build step even in package that don't currently need it.
+
+## entrypoint
+
+This is usually an `index.js` file which contains a wildcard export like,
+
+```js
+// eslint-disable-next-line import/export -- just types
+export * from './src/types.js';
+```
+
+The `types.js` file either defines the types itself or is an empty file (described above) paired with a `.d.ts` or `.ts` twin.
+
+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.
+
 ## exported.js
 
 The `exported.js` re-exports types into global namespace, for consumers that expect these to
 be ambient. This could be called "ambient.js" but we retain the filename for backwards compatibility.
 
 The pattern is to make these two files like this at package root:
 
-
 `exported.js`
 
 ```ts
@@ -44,7 +86,6 @@ Why the _ prefix? Because without it TS gets confused between the
 import and export symbols. ([h/t](https://stackoverflow.com/a/66588974))
 Note one downside vs ambients is that these types will appear to be on `globalThis`.
 
-
 ## Generating API docs
 
 We use [TypeDoc](https://typedoc.org/) to render API docs in HTML.
@@ -53,4 +94,3 @@ We use [TypeDoc](https://typedoc.org/) to render API docs in HTML.
 yarn docs
 open api-docs/index.html
 ```
-
diff --git a/packages/ERTP/src/index.js b/packages/ERTP/src/index.js
@@ -1,4 +1,5 @@
 // @jessie-check
+/// <reference types="@agoric/internal/exported" />
 
 export * from './amountMath.js';
 export * from './issuerKit.js';

diff --git a/packages/ERTP/src/types.js b/packages/ERTP/src/types.js
@@ -3,7 +3,7 @@
 // Ensure this is a module.
 export {};
 
-/// <reference types="ses"/>
+/// <reference types="ses" />
 /**
  * @import {Passable, RemotableObject} from '@endo/pass-style')
  * @import {CopyBag, CopySet, Key} from '@endo/patterns')

diff --git a/packages/SwingSet/package.json b/packages/SwingSet/package.json
@@ -101,6 +101,6 @@
     "access": "public"
   },
   "typeCoverage": {
-    "atLeast": 74.99
+    "atLeast": 75.02
   }
 }
diff --git a/packages/SwingSet/src/vats/timer/types.js b/packages/SwingSet/src/vats/timer/types.js
diff --git a/packages/agoric-cli/src/publish.js b/packages/agoric-cli/src/publish.js
@@ -1,5 +1,5 @@
 // @ts-check
-/// <reference types="ses"/>
+/// <reference types="ses" />
 
 import { E } from '@endo/far';
 

diff --git a/packages/assert/src/types-ambient.js b/packages/assert/src/types-ambient.js
@@ -1,5 +1,5 @@
 // @ts-check
-/// <reference types="ses"/>
+/// <reference types="ses" />
 
 // Based on
 // https://github.com/endojs/endo/blob/HEAD/packages/ses/src/error/types.js

diff --git a/packages/base-zone/package.json b/packages/base-zone/package.json
@@ -57,6 +57,6 @@
     "workerThreads": false
   },
   "typeCoverage": {
-    "atLeast": 89.87
+    "atLeast": 90.77
   }
 }
diff --git a/packages/boot/package.json b/packages/boot/package.json
@@ -91,6 +91,6 @@
     "workerThreads": false
   },
   "typeCoverage": {
-    "atLeast": 86.47
+    "atLeast": 86.74
   }
 }
diff --git a/packages/builders/package.json b/packages/builders/package.json
@@ -79,6 +79,6 @@
     "workerThreads": false
   },
   "typeCoverage": {
-    "atLeast": 74.28
+    "atLeast": 74.23
   }
 }
diff --git a/packages/cache/src/main.js b/packages/cache/src/main.js
@@ -1,7 +1,5 @@
 // @jessie-check
-
-// XXX ambient types runtime imports until https://github.com/Agoric/agoric-sdk/issues/6512
-import '@agoric/internal/exported.js';
+/// <reference types="@agoric/internal/exported" />
 
 // eslint-disable-next-line import/export
 export * from './types.js';

diff --git a/packages/casting/package.json b/packages/casting/package.json
@@ -24,7 +24,6 @@
   "dependencies": {
     "@agoric/internal": "^0.3.2",
     "@agoric/notifier": "^0.6.2",
-    "@agoric/spawner": "^0.6.8",
     "@agoric/store": "^0.9.2",
     "@cosmjs/encoding": "^0.32.3",
     "@cosmjs/proto-signing": "^0.32.3",

diff --git a/packages/casting/src/follower-cosmjs.js b/packages/casting/src/follower-cosmjs.js
@@ -1,4 +1,4 @@
-/// <reference types="ses"/>
+/// <reference types="ses" />
 
 import { E, Far } from '@endo/far';
 import * as tendermint34 from '@cosmjs/tendermint-rpc';

diff --git a/packages/casting/src/main.js b/packages/casting/src/main.js
@@ -1,6 +1,6 @@
 // @jessie-check
 
-import '@agoric/internal/exported.js';
+/// <reference types="@agoric/internal/exported" />
 
 // eslint-disable-next-line import/export
 export * from './types.js'; // no named exports

diff --git a/packages/deploy-script-support/src/helpers.js b/packages/deploy-script-support/src/helpers.js
@@ -1,6 +1,5 @@
 // @ts-check
 
-/// <reference types="../../time/src/types.js" />
 /// <reference path="../../zoe/exported.js" />
 
 import { E } from '@endo/far';

diff --git a/packages/governance/package.json b/packages/governance/package.json
@@ -34,7 +34,6 @@
     "@agoric/assert": "^0.6.0",
     "@agoric/ertp": "^0.16.2",
     "@agoric/internal": "^0.3.2",
-    "@agoric/network": "^0.1.0",
     "@agoric/notifier": "^0.6.2",
     "@agoric/store": "^0.9.2",
     "@agoric/time": "^0.3.2",

diff --git a/packages/governance/src/index.js b/packages/governance/src/index.js
@@ -1,10 +1,8 @@
+/// <reference types="@agoric/internal/exported" />
+/// <reference types="@agoric/ertp/exported" />
 // XXX ambient types runtime imports until https://github.com/Agoric/agoric-sdk/issues/6512
-import '@agoric/internal/exported.js';
-import '@agoric/ertp/exported.js';
 import '@agoric/zoe/exported.js';
 
-/// <reference path="./types-ambient.js" />
-
 export {
   ChoiceMethod,
   ElectionType,

diff --git a/packages/inter-protocol/src/auction/auctionBook.js b/packages/inter-protocol/src/auction/auctionBook.js
@@ -1,6 +1,7 @@
+/// <reference types="@agoric/internal/exported" />
+/// <reference types="@agoric/governance/exported" />
+
 // XXX ambient types runtime imports until https://github.com/Agoric/agoric-sdk/issues/6512
-import '@agoric/internal/exported.js';
-import '@agoric/governance/exported.js';
 import '@agoric/zoe/exported.js';
 import '@agoric/zoe/src/contracts/exported.js';
 

diff --git a/packages/inter-protocol/src/auction/auctioneer.js b/packages/inter-protocol/src/auction/auctioneer.js
@@ -1,4 +1,4 @@
-import '@agoric/governance/exported.js';
+/// <reference types="@agoric/governance/exported" />
 import '@agoric/zoe/exported.js';
 import '@agoric/zoe/src/contracts/exported.js';
 

diff --git a/packages/inter-protocol/src/auction/scheduleMath.js b/packages/inter-protocol/src/auction/scheduleMath.js
@@ -4,6 +4,8 @@ import { TimeMath } from '@agoric/time';
 import { natSafeMath } from '@agoric/zoe/src/contractSupport/index.js';
 import { assertAllDefined, makeTracer } from '@agoric/internal';
 
+/** @import {TimestampRecord} from '@agoric/time'; */
+
 const { subtract, multiply, floorDivide } = natSafeMath;
 const { Fail } = assert;
 
@@ -35,7 +37,7 @@ const subtract1 = relTime =>
  * their collateral).
  *
  * @param {Awaited<import('./params.js').AuctionParamManager>} params
- * @param {Timestamp} baseTime
+ * @param {TimestampRecord} baseTime
  * @returns {import('./scheduler.js').Schedule}
  */
 export const computeRoundTiming = (params, baseTime) => {
@@ -83,7 +85,7 @@ export const computeRoundTiming = (params, baseTime) => {
   // computed start is `startDelay + baseTime + freq - (baseTime mod freq)`.
   // That is, if there are hourly starts, we add an hour to the time, and
   // subtract baseTime mod freq. Then we add the delay.
-  /** @type {import('@agoric/time').TimestampRecord} */
+  /** @type {TimestampRecord} */
   const startTime = TimeMath.addAbsRel(
     TimeMath.addAbsRel(
       baseTime,

diff --git a/packages/inter-protocol/src/econCommitteeCharter.js b/packages/inter-protocol/src/econCommitteeCharter.js
@@ -1,6 +1,6 @@
 // @jessie-check
+/// <reference types="@agoric/governance/exported" />
 
-import '@agoric/governance/exported.js';
 import { M, mustMatch } from '@agoric/store';
 import { TimestampShape } from '@agoric/time';
 import { prepareExo, provideDurableMapStore } from '@agoric/vat-data';

diff --git a/packages/inter-protocol/src/index.js b/packages/inter-protocol/src/index.js
@@ -1,3 +1,4 @@
 // @jessie-check
+/// <reference types="@agoric/vats/src/core/types-ambient" />
 
 export { calculateCurrentDebt } from './interest-math.js';
diff --git a/packages/inter-protocol/src/proposals/econ-behaviors.js b/packages/inter-protocol/src/proposals/econ-behaviors.js
@@ -1,8 +1,5 @@
 // @jessie-check
 
-// XXX ambient types runtime imports until https://github.com/Agoric/agoric-sdk/issues/6512
-import '@agoric/vats/src/core/types-ambient.js';
-
 import { AmountMath } from '@agoric/ertp';
 import { deeplyFulfilledObject, makeTracer } from '@agoric/internal';
 import { makeStorageNodeChild } from '@agoric/internal/src/lib-chainStorage.js';

diff --git a/packages/inter-protocol/src/psm/psm.js b/packages/inter-protocol/src/psm/psm.js
@@ -1,7 +1,7 @@
 // @jessie-check
+/// <reference types="@agoric/governance/exported" />
 
 // XXX ambient types runtime imports until https://github.com/Agoric/agoric-sdk/issues/6512
-import '@agoric/governance/exported.js';
 import '@agoric/zoe/exported.js';
 import '@agoric/zoe/src/contracts/exported.js';
 

diff --git a/packages/inter-protocol/src/vaultFactory/vaultDirector.js b/packages/inter-protocol/src/vaultFactory/vaultDirector.js
@@ -1,8 +1,7 @@
+/// <reference types="@agoric/governance/exported" />
 import '@agoric/zoe/exported.js';
 import '@agoric/zoe/src/contracts/exported.js';
 
-import '@agoric/governance/exported.js';
-
 import { AmountMath, AmountShape, BrandShape, IssuerShape } from '@agoric/ertp';
 import {
   GovernorFacetShape,

diff --git a/packages/inter-protocol/src/vaultFactory/vaultFactory.js b/packages/inter-protocol/src/vaultFactory/vaultFactory.js
@@ -1,7 +1,7 @@
 // @jessie-check
+/// <reference types="@agoric/governance/exported" />
 
 // XXX ambient types runtime imports until https://github.com/Agoric/agoric-sdk/issues/6512
-import '@agoric/governance/exported.js';
 import '@agoric/zoe/exported.js';
 import '@agoric/zoe/src/contracts/exported.js';
 

diff --git a/packages/inter-protocol/test/auction/scheduleMath.test.js b/packages/inter-protocol/test/auction/scheduleMath.test.js
@@ -211,8 +211,8 @@ const TWO_PM = 1680876000n;
 const FIVE_MINUTES = 5n * 60n;
 const FIFTEEN_MINUTES = 15n * 60n;
 const defaults = /** @type {any} */ (makeDefaultParams());
-const TWO_PM_SCHED = computeRoundTiming(defaults, TWO_PM - 1n);
-const THREE_PM_SCHED = computeRoundTiming(defaults, TWO_PM);
+const TWO_PM_SCHED = computeRoundTiming(defaults, coerceAbs(TWO_PM - 1n));
+const THREE_PM_SCHED = computeRoundTiming(defaults, coerceAbs(TWO_PM));
 
 const checkDescendingStep = (t, liveSchedule, nextSchedule, now, expected) => {
   const nowTime = coerceAbs(now);

diff --git a/packages/inter-protocol/test/psm/gov-replace-committee.js b/packages/inter-protocol/test/psm/gov-replace-committee.js
@@ -1,6 +1,6 @@
 /* global E */
 // @ts-check
-/// <reference types="@agoric/vats/src/core/core-eval-env"/>
+/// <reference types="@agoric/vats/src/core/core-eval-env" />
 /**
  * @file Script to replace the econ governance committee in a SwingSet Core Eval
  *   (aka big hammer)

diff --git a/packages/internal/src/index.js b/packages/internal/src/index.js
@@ -1,6 +1,6 @@
 // @jessie-check
 
-/// <reference types="ses"/>
+/// <reference types="ses" />
 
 export * from './config.js';
 export * from './debug.js';

diff --git a/packages/network/package.json b/packages/network/package.json
@@ -70,6 +70,6 @@
     "workerThreads": false
   },
   "typeCoverage": {
-    "atLeast": 89.5
+    "atLeast": 89.39
   }
 }
diff --git a/packages/notifier/src/asyncIterableAdaptor.js b/packages/notifier/src/asyncIterableAdaptor.js
@@ -1,4 +1,4 @@
-/// <reference types="ses"/>
+/// <reference types="ses" />
 
 import { E } from '@endo/far';
 import { subscribeLatest } from './subscribe.js';

diff --git a/packages/notifier/src/index.js b/packages/notifier/src/index.js
@@ -1,7 +1,5 @@
 // @jessie-check
-
-// XXX ambient types runtime imports until https://github.com/Agoric/agoric-sdk/issues/6512
-import '@agoric/internal/exported.js';
+/// <reference types="@agoric/internal/exported" />
 
 export {
   makePublishKit,

diff --git a/packages/notifier/src/notifier.js b/packages/notifier/src/notifier.js
@@ -1,4 +1,4 @@
-/// <reference types="ses"/>
+/// <reference types="ses" />
 
 import { assert } from '@agoric/assert';
 import { E, Far } from '@endo/far';

diff --git a/packages/notifier/src/publish-kit.js b/packages/notifier/src/publish-kit.js
@@ -1,4 +1,4 @@
-/// <reference types="ses"/>
+/// <reference types="ses" />
 
 import { canBeDurable, prepareExoClassKit } from '@agoric/vat-data';
 import { E, Far } from '@endo/far';

diff --git a/packages/notifier/src/subscriber.js b/packages/notifier/src/subscriber.js
@@ -1,6 +1,6 @@
 // @jessie-check
 
-/// <reference types="ses"/>
+/// <reference types="ses" />
 
 import { E, Far } from '@endo/far';
 import { subscribeEach } from './subscribe.js';

diff --git a/packages/orchestration/index.js b/packages/orchestration/index.js
@@ -1,3 +1,9 @@
+/// <reference types="@agoric/internal/exported" />
+/// <reference types="@agoric/vats/src/core/types-ambient" />
+
+// XXX ambient types runtime imports until https://github.com/Agoric/agoric-sdk/issues/6512
+import '@agoric/zoe/exported.js';
+
 export * from './src/types.js';
 export * from './src/service.js';
 export * from './src/typeGuards.js';
diff --git a/packages/orchestration/package.json b/packages/orchestration/package.json
@@ -83,6 +83,6 @@
     "access": "public"
   },
   "typeCoverage": {
-    "atLeast": 96.18
+    "atLeast": 97.25
   }
 }
diff --git a/packages/pegasus/package.json b/packages/pegasus/package.json
@@ -72,6 +72,6 @@
     "access": "public"
   },
   "typeCoverage": {
-    "atLeast": 90.68
+    "atLeast": 90.61
   }
 }