Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some Playwright types now reported as any after upgrading to 5.7.2 #60619

Open
mrmckeb opened this issue Nov 26, 2024 · 4 comments
Open

Some Playwright types now reported as any after upgrading to 5.7.2 #60619

mrmckeb opened this issue Nov 26, 2024 · 4 comments
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Milestone

Comments

@mrmckeb
Copy link

mrmckeb commented Nov 26, 2024

πŸ”Ž Search Terms

I manually scrolled through all issues reported since the release of 5.7.2.

πŸ•— Version & Regression Information

  • This changed between versions 5.6.3 and 5.7.2

⏯ Playground Link

https://github.com/mrmckeb/pw-repro

πŸ’» Code

Minimal repro available here:
https://github.com/mrmckeb/pw-repro

The Playwright team are also able to reproduce:
microsoft/playwright#33763

The affected code (from the repro) is:

import { test as base, expect } from "@playwright/test";

const test = base.extend({
  // baseUrl, page, use are now `any`
  page: async ({ baseURL, page }, use) => {
    await page.goto(baseURL);
    await use(page);
  },
});

πŸ™ Actual behavior

Types that were previously available are now typed as any. JSDoc continues to work as expected.

πŸ™‚ Expected behavior

Types behave as they did in TypeScript 5.6.3.

Additional information about the issue

We found this issue when upgrading from 5.6.3 to 5.7.2. I couldn't find a related issue here, so raised it at the Playwright repo to at least verify that they could reproduce and that they didn't think it was an issue with their types.

The Playwright team also confirmed that they have 71 new errors across 41 files when moving to TypeScript 5.7.2.

@Andarist
Copy link
Contributor

bisects to #52095 , minimal-ish repro case: TS playground

I already have a somewhat good grasp of the issue. I'll write more about it tomorrow though and continue the investigation.

@Andarist
Copy link
Contributor

I don't understand why Exclude<R, Function> is there but I can also see how it could potentially mess things up (although I wasn't yet able internally verify why it messes it up). Unless I'm missing some constraint that those types try to apply there I think this is actually incorrect.

I'll dive deeper into it later but in the meantime I've also created a different repro case that doesn't include that Exclude and I think it's better to focus on it first: TS playground

@dgozman
Copy link

dgozman commented Nov 27, 2024

@Andarist The Exclude<R, Function> construct is to allow the fixture value in a literal form, e.g. page: 'foo', in addition to the fixture function like page: async ({}, use) => use('foo'). However, if the fixture type F is a function, the idea is to only allow the latter form, to avoid any possibility of the confusion between F and async ({}, use) => {...}.

By the way, I once tried to replace [K in keyof T]?: with [K in Exclude<keyof T, keyof PT>]?: to avoid the clash between T and PT properties. However, that breaks when R or Exclude<R, Function> is allowed in the TestFixture type, because it infers T in extend<T extends KeyValue> as KeyValue and every property becomes any.

@Andarist
Copy link
Contributor

@Andarist The Exclude<R, Function> construct is to allow the fixture value in a literal form, e.g. page: 'foo', in addition to the fixture function like page: async ({}, use) => use('foo'). However, if the fixture type F is a function, the idea is to only allow the latter form, to avoid any possibility of the confusion between F and async ({}, use) => {...}.

Ah ye, I already figured this part out (side note - you don't have any type tests in Playwright for this scenario, no test fail after replacing Exclude<R, Function> with just R there).

I'm not yet sure where it breaks internally but, in general, "not a type" isn't something greatly supported by TS. I put together some notes~ on various ways to do it in this TS playground. Ultimately, only your strategy gives the desired results here.

By the way, I once tried to replace [K in keyof T]?: with [K in Exclude<keyof T, keyof PT>]?: to avoid the clash between T and PT properties. However, that breaks when R or Exclude<R, Function> is allowed in the TestFixture type, because it infers T in extend as KeyValue and every property becomes any.

You are on the right track here when it comes to the root cause. The problem is that your PT is already a resolved mapped type with function properties. But then T is being inferred into from the same object that is typed using an intersection over mapped types applied to T and PT and it really doesn't know that it shouldn't use page property because it's already part of PT.

With my change the compiler uses all property types that could come out of deferred instantiations at this position. So it uses function types from all of those 4 mapped types and they are just not compatible with each other. That's how we end up with implicit anys.

To be fair, even before TS 5.7 the inference didn't quite work OK in your case (the contextual parameter types did though): TS playground. I somewhat doubt you care about it, when defining extra fixtures it's advised to use type params explicitly here instead of relying on the inference. It would certainly be great to make it work though.

Luckily-ish since TS 5.6 (since this PR) we can leverage filtering mapped types to disambiguite slightly between those mapped types: TS playground

And when it comes to the inference, the situation could be improved by this PR. We can see some improvements already on the TS playground using a TS build from that PR: TS playground. Based on those findings, I need to take a look at what happens with context-sensitive functions there because they seem to brak this inference.

Note that on the TS playground above I also replaced multiple extends KeyValue with extends {}. This is a safer default because TS uses the constraint when there are no inference candidates. Through extends KeyValue anys can easily creep into the user types and I think it's best to avoid that risk.

TLDR: I created a PR with a fix for this in Playwright here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted You can do this Possible Improvement The current behavior isn't wrong, but it's possible to see that it might be better in some cases
Projects
None yet
Development

No branches or pull requests

4 participants