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

Validate types with tsc without skipLibCheck option + introduce type declaration checking #38

Merged
merged 8 commits into from
May 21, 2024

Conversation

cmdoptesc
Copy link
Contributor

@cmdoptesc cmdoptesc commented Apr 23, 2024

Hi Vova, I experienced a number of type errors when I imported the @anycable/web package into my codebase, but didn't notice the errors when I pulled down anycable-client and ran yarn tsc. Realized it was because skipLibCheck in tsconfig.json had skipped checking the type declaration files (*.d.ts) in this repo.

  • fixed all the errors by disabling the skipLibCheck flag
  • still need skipLibCheck for check-dts to pass due to errors from AbortController and AbortSignal, so added it back to tsconfig.json after I was done
  • added a separate typecheck with tsc using a different config (noskiplibcheck_tsconfig.json) that will check the type declarations in this repo

@cmdoptesc cmdoptesc force-pushed the fix-types branch 4 times, most recently from 5e63c8e to 0e8f9ab Compare April 26, 2024 18:57
@palkan
Copy link
Member

palkan commented Apr 26, 2024

Great! Thanks!

Could you please take a look at CI failures?

Most types were already defined, but just not correctly imported
CreateOptions was expecting a type for its generic

Also export ProtocolID from @anycable/core
@cmdoptesc
Copy link
Contributor Author

@palkan is there a way for me to kick off another CI run, or can only you do that?

@palkan
Copy link
Member

palkan commented Apr 26, 2024

is there a way for me to kick off another CI run, or can only you do that?

It's only me for first-time PRs; as soon as we get this merged, all your subsequent PRs will run automatically.

@palkan
Copy link
Member

palkan commented Apr 26, 2024

Looks like we need to add some configuration to ignore node_modules/**/* and **/error.ts files from type checking.

@cmdoptesc
Copy link
Contributor Author

cmdoptesc commented Apr 26, 2024

Hmmm, I had trouble trying to repro the type failures on the CI run because my local tests can't get that far in the process due to import issues. I think the type failures on CI stem from the check-dts step.

That didn't fail before because skipLibCheck was included.

Because we only care about the outputs from the packages directory, I propose we change the check to check-dts 'packages/**/*'. What do you think?

I can repro now with yarn check-dts.

Those type failures are caused by swapping the position of the [Parameters<ActionsType[A]>[0]] type (e0224d2). I'm assuming that the behavior modeled in channel/errors.ts is correct and my changes were wrong. But I also don't think the existing declaration was correct because there was a type error prior to the swap:

✖ packages/core/channel/index.d.ts:73:21: Type error TS2344
  Type 'ActionsType[A]' does not satisfy the constraint '(...args: any) => any'.

I'll try to look into that error more this afternoon, but if you have any spare time on your end, I'd love a second set of eyes on fixing this declaration:

perform<A extends keyof ActionsType>(

Fixed the declaration with the second-to-last commit.

We'll also need to figure out a way to ignore the globals.d.ts AbortController errors.

Seems the above is common enough in check-nts that their FAQ addresses it by adding skipLibCheck 😕 :
https://github.com/ai/check-dts/blob/7020f61159798fbe19d2a1e38aadd75ab0c7ccc5/README.md#i-am-getting-an-error-from-node-types-how-do-i-skip-node_modules

Protocol's reset parameter is typed `ReasonError`, a superset of `Error`,
so ActionCableProtocol should use the same type
With `skipLibCheck` omitted, Channel#perform would fail tsc with:

```
Type 'ActionsType[A]' does not satisfy the constraint '(...args: any) => any'.
  Type 'ActionsType[keyof ActionsType]' is not assignable to type '(...args: any) => any'.
    Type 'ActionsType[string] | ActionsType[number] | ActionsType[symbol]' is not assignable to type '(...args: any) => any'.
```

This is because `Parameters` expects to take in a function, but the
existing ternary doesn't ensure that it's being called on a function.

Parameters: https://www.typescriptlang.org/docs/handbook/utility-types.html#parameterstype

So we should use the `ActionsType[A] extends () => void` to guard
for a function, but that was too strict because it would only match
against functions that took _no_ parameters.

If we expand the type to take any function with any amount of
parameters, then we can safely call `Parameters` on `ActionsType`.
And we can use the `[]` at the very end to accept any other type
`ActionsType` might be.

Co-authored-by: throwandgo <[email protected]>
@cmdoptesc
Copy link
Contributor Author

@palkan can I please trouble you to kick off another CI run?

Also, the last commit with introducing a secondary typescript config file may be a bit controversial. I wish the logic was inverted where check-dts could take in an explicit option to skip declaration files, and tsc could just rely on the default tsconfig.json file.

@cmdoptesc cmdoptesc changed the title Fix some types and remove skipLibCheck option Validate types with tsc without skipLibCheck option + introduce type declaration checking Apr 30, 2024
It looks like in tsconfig.json, the `skipLibCheck` flag was added
so that check-dts could ignore these external errors:

```
✖ node_modules/@types/node/globals.d.ts:67:13: Type error TS2502
  'AbortController' is referenced directly or indirectly in its own type annotation.

✖ node_modules/@types/node/globals.d.ts:74:13: Type error TS2502
  'AbortSignal' is referenced directly or indirectly in its own type annotation.
```

See the check-dts FAQ for their guidance:
https://github.com/ai/check-dts/blob/7020f61159798fbe19d2a1e38aadd75ab0c7ccc5/README.md#i-am-getting-an-error-from-node-types-how-do-i-skip-node_modules

But adding that flag meant the type declaration files (*.d.ts) in
this anycable-client repo were skipped, so any errors were
overlooked. This has downstream effects for any consumers of the
anycable-core or anycable-web packages. If any declaration files
had type errors (as they did), and the consumers did not set
`skipLibCheck` in their tsconfig, then they'd experience
a failure in their repo as evidenced in issue #39.

So it's prudent for us to _not_ skip checking the type declarations.

`tsc` and `check-dts` both read from tsconfig.json, but we can point
`tsc` to a different configuration file that omits `skipLibCheck`
and ensures this repo's declaration files are checked.

Added the tsc type check at the front because it's the quickest test,
so tests will fail early and fast if there's an error.
@palkan
Copy link
Member

palkan commented May 21, 2024

Thanks!

Upgrading @types/node fixed the problem with check-dts, so we don't need a separate config.

@palkan palkan merged commit 2a7752b into anycable:master May 21, 2024
4 checks passed
@cmdoptesc cmdoptesc deleted the fix-types branch May 22, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants