anycable-client icon indicating copy to clipboard operation
anycable-client copied to clipboard

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

Open cmdoptesc opened this issue 1 year ago • 6 comments

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 avatar Apr 23 '24 20:04 cmdoptesc

Great! Thanks!

Could you please take a look at CI failures?

palkan avatar Apr 26 '24 19:04 palkan

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

cmdoptesc avatar Apr 26 '24 19:04 cmdoptesc

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 avatar Apr 26 '24 20:04 palkan

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

palkan avatar Apr 26 '24 20:04 palkan

~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 (e0224d2e). 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:~ https://github.com/anycable/anycable-client/blob/9512487dec3780dd1d5125537479b6e392010d3b/packages/core/channel/index.d.ts#L67

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

cmdoptesc avatar Apr 26 '24 21:04 cmdoptesc

@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 avatar Apr 30 '24 04:04 cmdoptesc

Thanks!

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

palkan avatar May 21 '24 23:05 palkan