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

cmdoptesc commented 7 months ago

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

palkan commented 7 months ago

Great! Thanks!

Could you please take a look at CI failures?

cmdoptesc commented 7 months ago

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

palkan commented 7 months ago

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 commented 7 months ago

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

cmdoptesc commented 7 months ago

~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 commented 7 months ago

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

palkan commented 6 months ago

Thanks!

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

anycable / anycable-client

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