Enable/fix test typing in multiple packages

[peter-hamilton]

Description

Enabled type checking in a batch of react packages. The type errors being caused in these tests were caused because typescript had no visibility on the global jest matchers (@shopify/react-testing/matchers) imported by sewing kit via tests/setup/tests.ts when running tests.

This was addressed by adding a single file src/tests/setup.ts that imported @shopify/react-testing/matchers.

Note: tests/setup/tests.ts could be imported directly by adding a tsconfig.json file to tests/ and adding it as a project reference in all relevant packages, but this adds additional complexity with no major benefit.

Type of change

• react-univeral-provider Patch: Bug (non-breaking change which fixes an issue)
• react-tracking-pixel Patch: Bug (non-breaking change which fixes an issue)
• react-intersection-observer Patch: Bug (non-breaking change which fixes an issue)
• react-i18n-universal-provider Patch: Bug (non-breaking change which fixes an issue)
• react-hydrate Patch: Bug (non-breaking change which fixes an issue)
• react-google-analytics Patch: Bug (non-breaking change which fixes an issue)
• react-csrf-universal-provider Patch: Bug (non-breaking change which fixes an issue)
• react-csrf Patch: Bug (non-breaking change which fixes an issue)
• react-cookie Patch: Bug (non-breaking change which fixes an issue)
• react-compose Patch: Bug (non-breaking change which fixes an issue)
• react-app-bridge-universal-provider Patch: Bug (non-breaking change which fixes an issue)

Checklist

• I have added a changelog entry, prefixed by the type of change noted above (Documentation fix and Test update does not need a changelog as we do not publish new version)

[vsumner]

Would include work instead? https://www.typescriptlang.org/tsconfig#include

I don't have a problem with this solution.

[BPScott]

I poked around with this on friday afternoon, got nowhere conclusive yet, but I've got some alternative ideas that I didn't have time to try.

On the up side, this works.
On the down side it seems to work because there's any old file in each project and that kicks TS enough to realize it needs to apply the global augments. These files are never included and ran at runtime at any point which means this feels like load-bearing spookyness to me. They might as well be called packages/react-app-bridge-universal-provider/burp.ts. I'd feel more comfortable with this approach if these specific files were executed at runtime too.

It looks like sewing-kit-next would ready the files in packages/BLAH/tests/setup/test.ts (matching how tests/setup/test.ts works at the workspace root). If you remove the matchers from the global test setup (tests/setup/test.ts and apply them on a per-project basis then would mean this runtime-behaviour was important.

Alternatively instead of adding config to each project i wonder if we could repeat the declare global inside a config/typescript/matchers.d.ts file and have those files included in the projects that need them (same as the other stuff in the config/typescript directory). I'm tempted to say that repetition of the declare block feels nicer than having to add a file that is never executed at runtime to each package as it has a smaller footprint.

[peter-hamilton]

It looks like sewing-kit-next would ready the files in packages/BLAH/tests/setup/test.ts (matching how tests/setup/test.ts works at the workspace root). If you remove the matchers from the global test setup (tests/setup/test.ts and apply them on a per-project basis then would mean this runtime-behaviour was important.

• The challenge here is that tests/setup/*.ts files are outside the rootdir (src/). It throws error TS6059 if included in tsconfig.json so you don't gain type-checking.

Alternatively instead of adding config to each project i wonder if we could repeat the declare global inside a config/typescript/matchers.d.ts file and have those files included in the projects that need them (same as the other stuff in the config/typescript directory). I'm tempted to say that repetition of the declare block feels nicer than having to add a file that is never executed at runtime to each package as it has a smaller footprint.

• This works (example). The example solution includes all matcher definitions for every test, even those that are unused. This matches how they're loaded during tests. Not sure if having a tests/setup/test.ts per package has significant benefits.

[BPScott]

Fantastic work, thanks for continuing to poke at this.

The challenge here is that tests/setup/*.ts files are outside the rootdir (src/). It throws error TS6059 if included in tsconfig.json so you don't gain type-checking.

Yeah, we could change it so for packages the root is the package folder (package/BLAH) rather than src (package/BLAH/src) but I think that just begets other changes, and it feels like that leads to a whole can of worms that i don't really want to think about right now.

Not sure if having a tests/setup/test.ts per package has significant benefits.

There's probably some cuteness around "only loading what you need for a given package" but my gut-feel is those gains are going to be negligible and not worth the extra complexity compared to doing all the setup once at the workspace level.

I think I'm leaning towards the idea that for now we shouldn't need per-package testing config and that it's better that we keep the test config stuff centralised at the workspace level (i.e. what we have now).

I reckon let's run with what you've got in ab388b0 - removing the per-package files, and having a config/typescript/matchers.d.ts. I think that's less strange than having file that are never called but are needed for the sake of type augments.

I wonder if you can remove the saddle-up matchers and replace the following to trigger the global augments:

import type {} from 'saddle-up/matchers';
import type {} from 'saddle-up/koa-matchers';

[BPScott]

🎉