Skip to content

Commit

Permalink
document and adopt best practices for using .ts for typedefs (#10158)
Browse files Browse the repository at this point in the history
refs: #10086

## Description

Document the practice established in #10149 into a Best Practice.

Adopt it in most packages. I only left these `types.d.ts`
-  swing-store because the file is only re-exports, nothing to typecheck
- smart-wallet because there's no entrypoint for the package


### Security Considerations
none

### Scaling Considerations
none

### Documentation Considerations
per se

### Testing Considerations
#10149 (comment) asked for more testing. This PR was originally intended to close the issue but the regression test is complicated enough and this PR has gotten big enough that I think it should land on its own.

### Upgrade Considerations
n/a
  • Loading branch information
mergify[bot] authored Sep 28, 2024
2 parents 0b1501a + 21cc42c commit 6909994
Show file tree
Hide file tree
Showing 42 changed files with 518 additions and 496 deletions.
45 changes: 29 additions & 16 deletions docs/typescript.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,41 +4,54 @@ Our use of TypeScript has to accommodate both .js development in agoric-sdk (whi

## Best practices

- `.d.ts` for types modules
### Exported types

- `.ts` for modules defining exported types
- package entrypoint(s) exports explicit types
- use `/** @import ` comments to import types without getting the runtime module

### Ambient types

- `.d.ts` for modules defining ambient types
- import types using [triple-slash reference](https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html#-reference-types-)
- for packages upon which other packages expect ambient types:
- `exported.js` supplies ambients
- don't use runtime imports to get types ([issue](https://github.com/Agoric/agoric-sdk/issues/6512))

## .d.ts modules
## .ts modules

We cannot use `.ts` files in 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.)

### Benefits

- 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.
- Only `.d.ts` files can be used in [triple-slash reference types](https://www.typescriptlang.org/docs/handbook/triple-slash-directives.html#-reference-types-)
The trick is to use `.ts` for defining types and then make them available in the packages using a `types-index` module that has both `.js` and `.d.ts` files.

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,
**Entrypoint (index.js)**
```js
// eslint-disable-next-line import/export
export * from './src/types-index.js'; // no named exports
```

**types-index.js**
```js
// Empty JS file to correspond with its .d.ts twin
export {};
```

### Lack of type checking
**types-index.d.ts**
```ts
// Export all the types this package provides
export * from './types.js';
export * from './other-types.js';
```

The actual type implementation is then written in `types.ts` and `other-types.ts` files (per the example above).
These files are never runtime imported as they are only linked through a `.d.ts` file.

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.
## d.ts modules

### Alternatives
We take on the complexity above of indirection because `.d.ts` files aren't checked. 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`.)

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.
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.

## entrypoint

Expand Down
2 changes: 1 addition & 1 deletion packages/async-flow/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
export * from './src/async-flow.js';
export * from './src/types.js';
export * from './src/types-index.js';
export { makeSharedStateRecord } from './src/endowments.js';
2 changes: 2 additions & 0 deletions packages/async-flow/src/types-index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// Export all the types this package provides
export * from './types.js';
File renamed without changes.
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable no-use-before-define */
import type { Passable } from '@endo/pass-style';
import type { Vow, VowTools } from '@agoric/vow';
import type { LogStore } from './log-store.js';
Expand Down Expand Up @@ -52,7 +53,7 @@ type Simplify<T> = { [KeyType in keyof T]: T[KeyType] } & {};
/**
* Convert an entire Guest interface into what the host will implement.
*/
type HostInterface<T> = {
export type HostInterface<T> = {
[K in keyof T]: T[K] extends CallableFunction
? HostOf<T[K]>
: T[K] extends Record<string, any>
Expand Down
3 changes: 2 additions & 1 deletion packages/boot/test/bootstrapTests/ibcClientMock.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,8 @@ import { heapVowE as E } from '@agoric/vow/vat.js';

/**
* @import {Connection, PortAllocator} from '@agoric/network';
* @import {FarRef, ERef} from '@agoric/vow';
* @import {FarRef} from '@agoric/internal';
* @import {ERef} from '@agoric/vow';
*/

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/internal/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export * from './typeCheck.js';
export * from './typeGuards.js';

// eslint-disable-next-line import/export -- just types
export * from './types.js';
export * from './types-index.js';

export { objectMap } from '@endo/common/object-map.js';
export { objectMetaMap } from '@endo/common/object-meta-map.js';
Expand Down
1 change: 1 addition & 0 deletions packages/internal/src/types-index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export * from './types.js';
File renamed without changes.
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-disable max-classes-per-file */
import type { ERef, RemotableBrand } from '@endo/eventual-send';
import type { Primitive } from '@endo/pass-style';
import type { Pattern } from '@endo/patterns';
import type { Callable } from './utils.js';

/**
Expand All @@ -11,7 +12,7 @@ export type TotalMap<K, V> = Omit<Map<K, V>, 'get'> & {
/** Returns the element associated with the specified key in the TotalMap. */
get: (key: K) => V;
};
export type TotalMapFrom<M extends Map> =
export type TotalMapFrom<M extends Map<any, any>> =
M extends Map<infer K, infer V> ? TotalMap<K, V> : never;

export declare class Callback<I extends (...args: any[]) => any> {
Expand Down
4 changes: 2 additions & 2 deletions packages/notifier/src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,5 +28,5 @@ export {
export * from './storesub.js';
export * from './stored-notifier.js';

// eslint-disable-next-line import/export -- doesn't know types
export * from './types.js';
// eslint-disable-next-line import/export
export * from './types-index.js'; // no named exports in JS
2 changes: 2 additions & 0 deletions packages/notifier/src/types-index.d.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// Export all the types this package provides
export * from './types.js';
2 changes: 2 additions & 0 deletions packages/notifier/src/types-index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
// Empty JS file to correspond with its .d.ts twin
export {};
Loading

0 comments on commit 6909994

Please sign in to comment.