fix(types): fixed `Fixtures` type to disambiguite between intersected mapped types

[Andarist]

fixes https://github.com/microsoft/playwright/issues/33763 context behind the change can be found here: https://github.com/microsoft/TypeScript/issues/60619#issuecomment-2503967944 cc @dgozman

[github-actions[bot]]

Test results for "tests 1"

5 failed :x: [playwright-test] › types.spec.ts:19:5 › should check types of fixtures @macos-latest-node18-1 :x: [playwright-test] › types.spec.ts:19:5 › should check types of fixtures @ubuntu-latest-node18-1 :x: [playwright-test] › types.spec.ts:19:5 › should check types of fixtures @ubuntu-latest-node20-1 :x: [playwright-test] › types.spec.ts:19:5 › should check types of fixtures @ubuntu-latest-node22-1 :x: [playwright-test] › types.spec.ts:19:5 › should check types of fixtures @windows-latest-node18-1

2 flaky

:warning: [webkit-library] › library/browsercontext-add-cookies.spec.ts:429:3 › should allow unnamed cookies @webkit-ubuntu-22.04-node18
:warning: [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

37167 passed, 650 skipped :heavy_check_mark::heavy_check_mark::heavy_check_mark:

Merge workflow run.

[Andarist]

I ran a different set of tests locally and missed this before opening the PR. I'll look into the reported failure soon. FWIW, the failures are related to this piece of code: https://github.com/microsoft/playwright/blob/f3ae940684dd26de1d4674d66dd98b8722b08d50/tests/playwright-test/types.spec.ts#L110-L117

[dgozman]

Thank you for the PR, this looks fantastic!

I ran a different set of tests locally and missed this before opening the PR. I'll look into the reported failure soon. FWIW, the failures are related to this piece of code:

I applied your suggestion to replace KeyValue with {} and that makes this test pass locally for me. Let's see what the bots think: https://github.com/microsoft/playwright/pull/33784. Also works in this playground.

[Andarist]

@dgozman I already included the constraint fix here too :) not in as many places though, I chose to be conservative with my change

[github-actions[bot]]

Test results for "tests 1"

9 failed :x: [installation tests] › playwright-test-plugin.spec.ts:35:5 › npm: @playwright/test plugin should work @package-installations-macos-latest :x: [installation tests] › playwright-test-plugin.spec.ts:49:5 › pnpm: @playwright/test plugin should work @package-installations-macos-latest :x: [installation tests] › playwright-test-plugin.spec.ts:63:5 › yarn: @playwright/test plugin should work @package-installations-macos-latest :x: [installation tests] › playwright-test-plugin.spec.ts:35:5 › npm: @playwright/test plugin should work @package-installations-ubuntu-latest :x: [installation tests] › playwright-test-plugin.spec.ts:49:5 › pnpm: @playwright/test plugin should work @package-installations-ubuntu-latest :x: [installation tests] › playwright-test-plugin.spec.ts:63:5 › yarn: @playwright/test plugin should work @package-installations-ubuntu-latest :x: [installation tests] › playwright-test-plugin.spec.ts:35:5 › npm: @playwright/test plugin should work @package-installations-windows-latest :x: [installation tests] › playwright-test-plugin.spec.ts:49:5 › pnpm: @playwright/test plugin should work @package-installations-windows-latest :x: [installation tests] › playwright-test-plugin.spec.ts:63:5 › yarn: @playwright/test plugin should work @package-installations-windows-latest

2 flaky

:warning: [webkit-library] › library/trace-viewer.spec.ts:1512:1 › should serve css without content-type @webkit-ubuntu-22.04-node18
:warning: [playwright-test] › ui-mode-test-watch.spec.ts:145:5 › should watch all @windows-latest-node18-1

37163 passed, 650 skipped :heavy_check_mark::heavy_check_mark::heavy_check_mark:

Merge workflow run.

[jakebailey]

Per the linked issue, this only works if downstream users use TS 5.6+, right?

[Andarist]

Per the linked issue, this only works if downstream users use TS 5.6+, right?

Yes. I don't think there is a suitable way to rewrite those types that would support older TS versions. typesVersions exist for a reason - but, I won't lie, they are not exactly easy to set up.

One idea to rewrite this would be to avoid reverse mapped type inference here somehow while preserving the contextual typing capabilities. Having that inference here isn't particularly desired and it doesn't quite work today anyway in this case.

[Andarist]

I came up with this beautiful hack that makes it work: TS playground. Further testing would be required to make sure that it doesn't break anything but it looks really promising to me.

The idea behind this workaround is that properties of remapping mapped types don't contribute to contextual typing. This matches the pre-5.7 behavior for this particular scenario. So by ensuring that our as clause remaps the property by adding a dummy symbol in its "output" we make it works again. Given this symbol is completely private, I think it won't cause much harm... although it could impact type portability for the declaration emit. That could likely be addressed by Omiting it in the returned type if it even gets inferred as T's property (in the playground above it doesn't so this portability argument might just not be relevant to this situation after al).

[dgozman]

@Andarist Let me play a bit with this beautiful hack 😄

That said, we have to support at least TypeScript 5.0, so I'd prefer to avoid typesVersions if possible. Given that, I've replaced keyof T as Exclude<...> with Exclude<keyof T, ...> in hope that would somewhat work in earlier TS versions. IIUC, that would not infer as good, but at least it may prevent any or wrong type being taken for overridden fixtures. Let's see how #33784 fares on the bots.

[Andarist]

That said, we have to support at least TypeScript 5.0, so I'd prefer to avoid typesVersions if possible.

I mean, typesVersions is how you can support many different TS versions by providing different typing files for each of them. So that's exactly what could be used here if we don't find a suitable workaround. But also, I don't really recommend it because it would raise the complexity of your setup considerably ;p

Given that, I've replaced keyof T as Exclude<...> with Exclude<keyof T, ...> in hope that would somewhat work in earlier TS versions. IIUC, that would not infer as good, but at least it may prevent any or wrong type being taken for overridden fixtures.

It turns out that I confused myself here altogether. Both keyof T as Exclude<...> and Exclude<keyof T, ...> should do the trick for TS 5.0+.

It's correct to say that keyof T as Exclude<...> only works since TS 5.6 but that's when that mapped type is not intersected with other types.

All of this works based on subtle fact that you have this mix of known/resolved properties with properties of a type that is still generic. In such an intersection case, TS (before 5.7) would just ignore the generic mapped part of the intersection when providing a contextual type and would only use the more "concrete" part of the type. This wasn't consistent because in non-intersection cases it wouldn't ignore the generic mapped part at all, it would use it.

Fixing that had an unfortunate effect of breaking your types. But at the same time, it means that it doesn't quite matter how you write those generic mapped types here... because pre TS 5.7 they will just be ignored when selecting the contextual type.

[dgozman]

Closing in favor of #33784, thank you for all the help!