-
Notifications
You must be signed in to change notification settings - Fork 215
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
clean up types modules #9364
Changes from all commits
97c57c7
416818b
b02c004
0985eeb
41460f5
5140008
85fb5ac
caad41f
b269707
6a6e595
d371cb3
73a1c11
f39d40f
b2306a8
c6c0a39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That would be great indeed. I would likely still want the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
``` | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -101,6 +101,6 @@ | |
"access": "public" | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 74.99 | ||
"atLeast": 75.02 | ||
} | ||
} |
This file was deleted.
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'; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -57,6 +57,6 @@ | |
"workerThreads": false | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 89.87 | ||
"atLeast": 90.77 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,6 +91,6 @@ | |
"workerThreads": false | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 86.47 | ||
"atLeast": 86.74 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -79,6 +79,6 @@ | |
"workerThreads": false | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 74.28 | ||
"atLeast": 74.23 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) => { | ||
Comment on lines
-38
to
43
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 @Chris-Hibbert what do you think about narrowing this type at this point? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
|
@@ -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, | ||
|
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'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -70,6 +70,6 @@ | |
"workerThreads": false | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 89.5 | ||
"atLeast": 89.39 | ||
} | ||
} |
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'; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,6 +83,6 @@ | |
"access": "public" | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 96.18 | ||
"atLeast": 97.25 | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,6 +72,6 @@ | |
"access": "public" | ||
}, | ||
"typeCoverage": { | ||
"atLeast": 90.68 | ||
"atLeast": 90.61 | ||
} | ||
} |
There was a problem hiding this comment.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.