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

clean up types modules #9364

Merged
merged 15 commits into from
May 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .github/workflows/test-all-packages.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
46 changes: 43 additions & 3 deletions docs/typescript.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct.


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.
Copy link
Member

Choose a reason for hiding this comment

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

I didn't realize that, yikes!


### 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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed!


## 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
Expand Down Expand Up @@ -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.
Expand All @@ -53,4 +94,3 @@ We use [TypeDoc](https://typedoc.org/) to render API docs in HTML.
yarn docs
open api-docs/index.html
```

1 change: 1 addition & 0 deletions packages/ERTP/src/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// @jessie-check
/// <reference types="@agoric/internal/exported" />

export * from './amountMath.js';
export * from './issuerKit.js';
Expand Down
2 changes: 1 addition & 1 deletion packages/ERTP/src/types.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down
2 changes: 1 addition & 1 deletion packages/SwingSet/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,6 @@
"access": "public"
},
"typeCoverage": {
"atLeast": 74.99
"atLeast": 75.02
}
}
2 changes: 0 additions & 2 deletions packages/SwingSet/src/vats/timer/types.js

This file was deleted.

2 changes: 1 addition & 1 deletion packages/agoric-cli/src/publish.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// @ts-check
/// <reference types="ses"/>
/// <reference types="ses" />

import { E } from '@endo/far';

Expand Down
2 changes: 1 addition & 1 deletion packages/assert/src/types-ambient.js
Original file line number Diff line number Diff line change
@@ -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
Expand Down
2 changes: 1 addition & 1 deletion packages/base-zone/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,6 @@
"workerThreads": false
},
"typeCoverage": {
"atLeast": 89.87
"atLeast": 90.77
}
}
2 changes: 1 addition & 1 deletion packages/boot/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,6 @@
"workerThreads": false
},
"typeCoverage": {
"atLeast": 86.47
"atLeast": 86.74
}
}
2 changes: 1 addition & 1 deletion packages/builders/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,6 @@
"workerThreads": false
},
"typeCoverage": {
"atLeast": 74.28
"atLeast": 74.23
}
}
4 changes: 1 addition & 3 deletions packages/cache/src/main.js
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
1 change: 0 additions & 1 deletion packages/casting/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 1 addition & 1 deletion packages/casting/src/follower-cosmjs.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/// <reference types="ses"/>
/// <reference types="ses" />

import { E, Far } from '@endo/far';
import * as tendermint34 from '@cosmjs/tendermint-rpc';
Expand Down
2 changes: 1 addition & 1 deletion packages/casting/src/main.js
Original file line number Diff line number Diff line change
@@ -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
Expand Down
1 change: 0 additions & 1 deletion packages/deploy-script-support/src/helpers.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// @ts-check

/// <reference types="../../time/src/types.js" />
/// <reference path="../../zoe/exported.js" />

import { E } from '@endo/far';
Expand Down
1 change: 0 additions & 1 deletion packages/governance/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
6 changes: 2 additions & 4 deletions packages/governance/src/index.js
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
5 changes: 3 additions & 2 deletions packages/inter-protocol/src/auction/auctionBook.js
Original file line number Diff line number Diff line change
@@ -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';

Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/auction/auctioneer.js
Original file line number Diff line number Diff line change
@@ -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';

Expand Down
6 changes: 4 additions & 2 deletions packages/inter-protocol/src/auction/scheduleMath.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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) => {
Comment on lines -38 to 43
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/econCommitteeCharter.js
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
1 change: 1 addition & 0 deletions packages/inter-protocol/src/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
// @jessie-check
/// <reference types="@agoric/vats/src/core/types-ambient" />

export { calculateCurrentDebt } from './interest-math.js';
3 changes: 0 additions & 3 deletions packages/inter-protocol/src/proposals/econ-behaviors.js
Original file line number Diff line number Diff line change
@@ -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';
Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/psm/psm.js
Original file line number Diff line number Diff line change
@@ -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';

Expand Down
3 changes: 1 addition & 2 deletions packages/inter-protocol/src/vaultFactory/vaultDirector.js
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
2 changes: 1 addition & 1 deletion packages/inter-protocol/src/vaultFactory/vaultFactory.js
Original file line number Diff line number Diff line change
@@ -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';

Expand Down
4 changes: 2 additions & 2 deletions packages/inter-protocol/test/auction/scheduleMath.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
2 changes: 1 addition & 1 deletion packages/internal/src/index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// @jessie-check

/// <reference types="ses"/>
/// <reference types="ses" />

export * from './config.js';
export * from './debug.js';
Expand Down
2 changes: 1 addition & 1 deletion packages/network/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,6 @@
"workerThreads": false
},
"typeCoverage": {
"atLeast": 89.5
"atLeast": 89.39
}
}
2 changes: 1 addition & 1 deletion packages/notifier/src/asyncIterableAdaptor.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/// <reference types="ses"/>
/// <reference types="ses" />

import { E } from '@endo/far';
import { subscribeLatest } from './subscribe.js';
Expand Down
4 changes: 1 addition & 3 deletions packages/notifier/src/index.js
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
2 changes: 1 addition & 1 deletion packages/notifier/src/notifier.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/// <reference types="ses"/>
/// <reference types="ses" />

import { assert } from '@agoric/assert';
import { E, Far } from '@endo/far';
Expand Down
2 changes: 1 addition & 1 deletion packages/notifier/src/publish-kit.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
/// <reference types="ses"/>
/// <reference types="ses" />

import { canBeDurable, prepareExoClassKit } from '@agoric/vat-data';
import { E, Far } from '@endo/far';
Expand Down
2 changes: 1 addition & 1 deletion packages/notifier/src/subscriber.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
// @jessie-check

/// <reference types="ses"/>
/// <reference types="ses" />

import { E, Far } from '@endo/far';
import { subscribeEach } from './subscribe.js';
Expand Down
6 changes: 6 additions & 0 deletions packages/orchestration/index.js
Original file line number Diff line number Diff line change
@@ -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';
2 changes: 1 addition & 1 deletion packages/orchestration/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,6 @@
"access": "public"
},
"typeCoverage": {
"atLeast": 96.18
"atLeast": 97.25
}
}
2 changes: 1 addition & 1 deletion packages/pegasus/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,6 @@
"access": "public"
},
"typeCoverage": {
"atLeast": 90.68
"atLeast": 90.61
}
}
Loading
Loading