Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.

Implement type checking #97

Open
turadg opened this issue Apr 6, 2022 · 4 comments
Open

Implement type checking #97

turadg opened this issue Apr 6, 2022 · 4 comments
Assignees
Labels
tooling repo-wide infrastructure UI

Comments

@turadg
Copy link
Member

turadg commented Apr 6, 2022

This repo has a TypeScript config (https://github.com/Agoric/dapp-treasury/blob/main/ui/jsconfig.json) but there's no command that uses it to verify types check out.

Acceptance criteria

  • dev command to check types
  • type errors fail CI

Examples of errors present

cd ui
yarn tsc -p jsconfig.json
# Found 35 errors in 21 files.

Duplicate ambients

../../agoric-sdk/node_modules/@endo/promise-kit/index.js:4:23 - error TS4090: Conflicting definitions for 'ses' found at '/opt/agoric/agoric-sdk/node_modules/ses/index.d.ts' and '/opt/agoric/dapp-treasury/node_modules/ses/index.d.ts'. Consider installing a specific version of this library to resolve the conflict.
  The file is in the program because:
    Imported via 'ses' from file '/opt/agoric/dapp-treasury/ui/src/install-ses-lockdown.js' with packageId 'ses/[email protected]'
    Type library referenced via 'ses' from file '/opt/agoric/dapp-treasury/ui/node_modules/@endo/eventual-send/src/handled-promise.js' with packageId 'ses/[email protected]'
    Type library referenced via 'ses' from file '/opt/agoric/agoric-sdk/node_modules/@endo/promise-kit/index.js' with packageId 'ses/[email protected]'

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

  src/install-ses-lockdown.js:1:8
    1 import 'ses';
             ~~~~~
    File is included via import here.
  node_modules/@endo/eventual-send/src/handled-promise.js:2:23
    2 /// <reference types="ses" />
                            ~~~
    File is included via type library reference here.

Missing types

../node_modules/@endo/captp/src/captp.js:32:15 - error TS2304: Cannot find name 'TrapGuest'.

32  * @property {TrapGuest} trapGuest if specified, enable this CapTP (guest) to
                 ~~~~~~~~~

../node_modules/@endo/captp/src/captp.js:35:15 - error TS2304: Cannot find name 'TrapHost'.

35  * @property {TrapHost} trapHost if specified, enable this CapTP (host) to serve
                 ~~~~~~~~

../node_modules/@endo/captp/src/captp.js:124:27 - error TS2304: Cannot find name 'CapTPSlot'.

124   /** @type {WeakMap<any, CapTPSlot>} */
                              ~~~~~~~~~
@samsiegart
Copy link
Contributor

Do you have any opinions on whether we should do this as a separate yarn command, or part of build/lint/test/etc.?

@turadg
Copy link
Member Author

turadg commented Apr 6, 2022

opinions

Almost always happy to opine :)

To make it easy for people to come into repo and be productive, I think the commands they're used to should work. E.g. running yarn tsc anywhere in the repo should do a typecheck because that's the TypeScript command for doing a typecheck.

Whether that gets run in a yarn lint, I'm weaker on. My mental model is that linting is disjoint from formatting and type checking, but I get that for some it encompasses all that. In sdk yarn lint runs both eslint and tsc. Here's how I think of these:

  • yarn format normalize anything that can't affect semantics (e.g. whitespace)
  • yarn lint static analysis of things that could require semantic change
  • yarn test execute code to observe its runtime

By that logic a typecheck wouldn't go under yarn test. Though I did that in https://github.com/endojs/endo/blob/88893be94254fca1821944fa730613cbdb29fbfd/packages/ses/package.json#L58 and maybe I should change it.

@turadg
Copy link
Member Author

turadg commented Jun 2, 2022

Upped priority to MN-1 because it'll help us get there faster by preventing bugs like Agoric/agoric-sdk#5260 (comment)

@turadg turadg self-assigned this Jun 2, 2022
@turadg
Copy link
Member Author

turadg commented Jun 3, 2022

There's a PR to enable this #120 but in its current state it passes CI by no longer resolving many types, leaving them as implicit any.

The motivation for this issue is that we can detect type incompatibilities during the test-dapp-treasury integration test. If we don't have types working correctly then CI isn't worth enabling.

So I'm expanding the scope of this to include that. I don't yet know what it'll take so I'm raising the estimate to be conservative. We'll know the costs better after discussing with @mhofman .

I'm leaving the MN-1 release tag here but I expect it may be deferred to 1.1.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
tooling repo-wide infrastructure UI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants