build: validate types

[timdeschryver]

From #577 (and #577 should be merged first to complete this PR)

It seems that the rollup plugin doesn't have an option to validate the types.
Some even created their own plugin to make this work, which seemed like an overkill to me and something we don't want to maintain.

I started looking at dtslint and tsd to validate the types after the build but this had some drawbacks.
dtslint expects a tsconfig at the root of the types.
For tsd we needed to copy-paste most of the compilerOptions tsconfig into our package.json and make some tweaks to the libs, and tsd also included the node_modules for some reason and there were also invalid types there...

Then I stumbled upon the --noemit option of tsc which does what it needs to do and is simple to implement and doesn't require an additional package. In this PR, we run tsc before the rollup build (without generating the output files).

[timdeschryver]

Skip checking node_modules, there's a problem with it because we include dom and webworker, both have identical types that clash. For example:

node_modules/typescript/lib/lib.webworker.d.ts:5951:13 - error TS2403: Subsequent variable declarations must have the same type.  Variable 'onrejectionhandled' must be of type '((this: Window, ev: PromiseRejectionEvent) => any) | null', but here has type '((this: DedicatedWorkerGlobalScope, ev: PromiseRejectionEvent) => any) | null'.

5951 declare var onrejectionhandled: ((this: DedicatedWorkerGlobalScope, ev: PromiseRejectionEvent) => any) | null;
                 ~~~~~~~~~~~~~~~~~~

  node_modules/typescript/lib/lib.dom.d.ts:19933:13
    19933 declare var onrejectionhandled: ((this: Window, ev: PromiseRejectionEvent) => any) | null;
                      ~~~~~~~~~~~~~~~~~~
    'onrejectionhandled' was also declared here.

[kettanaito]

Doesn't skipLibCheck: true negates all type checking originating from third-party libraries?

Can you please verify that we still get type violations if we use some third-party package against its defined types?

[timdeschryver]

You're right @kettanaito, third party type violations will not be caught.
See docs

If we want to keep this enabled, I think the only solution would be to create multiple tsconfig's.
One for each build, so they can have their own included libs.
This way they won't interfere with each other.

Or perhaps we could exclude typescript's definition files.

[kettanaito]

TS configs per each build target sound like a sensible idea. We won't have to compose TS configs in rollup.config.ts. I think it's still possible to provide tsc with a custom config like:

$ tsc --config <CONFIG_PATH>

[timdeschryver]

Adding those declaration files to our excludes doesn't help.
How do you want to proceed with this? Should we try a setup with multiple tsconfig files?
EDIT: I didn't notice your comment 😅

[kettanaito]

Let's try multiple tsconfig.json configurations. If you feel it starts to get messy and takes too much time, we can always discuss this and find a different solution. Thank you for your investigation on this, by the way!

[timdeschryver]

I'm confused.

I thought it wouldn't be able to add multiple tsconfigs because they are both (dom and webworker) included in our bundle (both are exported via the index file). Now the weird part is that I was able to exclude the webworker types after I modified some types in 2b59cd9.

After this, everything worked, but I don't exactly know why...

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2b59cd9:

Sandbox	Source
MSW React	Configuration

[kettanaito]

The listener doesn't have to accept MessageEvent only. This is a general shorthand function to create/remove event listeners on any target:

context.events.addListener(window, 'beforeunload')

I'd expect this call signature to throw type violations.

[kettanaito]

So removing the webworker lib tsc --no-emit now validates the types?

I wonder what types we lose by removing that lib. I'd suspect it has something to do with web workers 😛

[timdeschryver]

So removing the webworker lib tsc --no-emit now validates the types

Correct. That command was also validating our types before this change, but it was throwing errors due to duplicate types in dom and webworker. See microsoft/TypeScript#20595 for more info.

In that issue, someone mentioned to add the types of webworker manually. I tried to do that, but the issue was already resolved by removing the webworker lib.

My guess is that we don't use any of the webworker specific types, although I'm not 100% sure of that.
Do you think this is better than using the skipLibCheck option?

[kettanaito]

We should be able to validate the typings via the yarn test:ts command. It compiles the usage examples (in test/typings) and the library's output with it.

[timdeschryver]

Thanks @kettanaito 👨‍🔬🧪

[kettanaito]

Thank you for the initial effort behind this, @timdeschryver. Honored to have you on the team!