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

export external types as modules #4560

Open
kriskowal opened this issue Feb 16, 2022 · 8 comments
Open

export external types as modules #4560

kriskowal opened this issue Feb 16, 2022 · 8 comments
Assignees
Labels
technical-debt tooling repo-wide infrastructure

Comments

@kriskowal
Copy link
Member

At time of writing, we need to use --maxNodeModulesJsDepth in tsc commands for lint:types scripts in most Agoric SDK packages because most of our packages do not provide type definitions and do use ambient types expressed in JSDoc comments.

The worst outcome of this is that tools like IDE LSP plugins for tsc can’t see the necessary command line flag, and that flag cannot be expressed in project files like jsconfig.json. Consequently, type checking may pass in CI but appear to be failing in IDE’s. Lesser bad outcomes include slow type checking performance. One small upside is that we don’t have to use a build step to keep exported type definitions in sync with the JavaScript code.

To eliminate this problem we will need to both migrate away from ambient types and add a build step to generate type definitions to most projects. Packages that do not have internally coherent types will need manually written type definitions.

Migrating away from ambient types involves adding export {} to any types.js or exported.js files that only contain JSDoc comments, and then replacing import 'types.js' invocations with use of /** @type {import('./types.js').T} */ style references for individual imported types.

It’s also necessary to replace the convention of exported.js with an entry in the main module of each package like:

// eslint-disable-next-line import/export
export * from './src/exported.js';

Such that /* @type {import('dep').T */ can be used to refer to all the types exported by package dep.

Migrating away from ambient types is a breaking change and requires conventional commit ceremony:

fix(package)!: Migrate away from ambient types

*BREAKING CHANGE*: Package no longer carries ambient types. All types must be expressly imported in type definitions like `/** @type {import('package').Type} */` and `import 'package/src/export.js'` will no longer work as expected.

That covers ambient types. Then, to remove --maxNodeModulesJsDepth, you will need to arrange a different tsc --build command with "noEmit": false in a jsconfig.build.json and TODO👉 the necessary incantations to put type definition files in the right places 👈TODO and add that build step to the "build" script in package.json TODO according to conventions yet to be established TODO.

@mhofman
Copy link
Member

mhofman commented Feb 16, 2022

For reference, the @endo/marshal package just went through this ceremony: endojs/endo#1025, and the corresponding changes to the agoric-sdk don't seem to be too painful: #4413

@Chris-Hibbert
Copy link
Contributor

For the record, we were asked to take steps to not compound the problem until there is a solution. AIUI, "not compounding the problem" means something like not adding more type declarations in types.js files, and instead adding jsdoc declarations with the relevant source code. I think the implication is that whenever we do that, those types are only visible to other files if they use /** @type {import('./types.js').T} */ style references.

Is that a correct summary?

@mhofman
Copy link
Member

mhofman commented Mar 1, 2022

I believe @warner just went through the steps for the SwingSet package of doing a hybrid model.

You can still have a .js file which only declares types, but to avoid ambient-ness it needs to include an export {} somewhere. For a hybrid / transition, that means 2 files, one ambient, one with exported types.

You may be able to have the file declaring exported types still import from './types.js' (the ambient file) if it needs to reference the types that have not yet been converted to explicit export/import. I'm also wondering if it might not be possible to move all type declarations to exported types, and change the ambient file declarations to only be composed of /** @typedef {import('./exported.js').ExportedType} ExportedType */ to make exported types ambient for the transition.

@turadg
Copy link
Member

turadg commented Sep 9, 2022

I run into this again and again. Most recent was having to run CI a couple extra times due to missed type "errors" from packages that were inspecting their dependencies and coming up with different answers due to #4620.

With the technique in #5972 I think this will be a day of work and should be scheduled. I've marked it for MN 1.1.

@turadg turadg changed the title Avoid using tsc maxNodeModulesJsDepth option Avoid using tsc maxNodeModuleJsDepth option Sep 19, 2022
This was referenced Sep 20, 2022
@turadg
Copy link
Member

turadg commented Sep 30, 2022

Running into this,

../../../node_modules/@endo/captp/src/captp.js:13:10 - error TS2305: Module '"@endo/promise-kit"' has no exported member 'isPromise'.

13 import { isPromise, makePromiseKit } from '@endo/promise-kit';
            ~~~~~~~~~

../../ERTP/src/paymentLedger.js:4:10 - error TS2305: Module '"@endo/promise-kit"' has no exported member 'isPromise'.

4 import { isPromise } from '@endo/promise-kit';
           ~~~~~~~~~

../../internal/src/utils.js:4:10 - error TS2305: Module '"@endo/promise-kit"' has no exported member 'isPromise'.

4 import { isPromise } from '@endo/promise-kit';
           ~~~~~~~~~

@turadg
Copy link
Member

turadg commented Oct 25, 2022

With #6369 landed, we are no longer using tsc maxNodeModuleJsDepth option. So the title of this issue is satisfied. I haven't closed it because the body is not: Migrating away from ambient types.

I hope we still do that so I propose retitling this ticket. WDYT @kriskowal ?

@kriskowal
Copy link
Member Author

Whatever works for you. Technically, making a new issue might be better for training our estimator, but I’m all for saving time by reusing a ticket.

@turadg
Copy link
Member

turadg commented Nov 3, 2022

Since we're using a new way of estimating, I'll repurpose.

@turadg turadg changed the title Avoid using tsc maxNodeModuleJsDepth option export external types as modules Nov 3, 2022
mergify bot added a commit that referenced this issue Oct 15, 2024
refs: #4560

## Description

Exports ERTP types from the package entrypoint.

Establishes a pattern for constant enums so the type name matches.

Review by commit

### Security Considerations
none

### Scaling Considerations
none

### Documentation Considerations
none

### Testing Considerations


### Upgrade Considerations
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical-debt tooling repo-wide infrastructure
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants