chore(ct): vue prop type of JsonObject

[sand4rt]

The props type has been changed so that code below throws a type error:

test('type error', async ({ mount }) => {
  const component = await mount(Button, {
    props: {
      submit: () => {} // Type '() => void' is not assignable to type 'JsonValue | undefined'
    },
  });
});

[dospunk]

If we can only pass JsonObjects to props, the how are we supposed to test components that take things like functions or more complex objects as props?

[sand4rt]

Vue works with events/emits instead of callbacks/functions as props. The await mount(Button, { on: { submit: () => {} }) is used for that. See: https://github.com/microsoft/playwright/blob/main/tests/components/ct-vue-vite/tests/events/events.spec.ts

What do you mean by complex objects?

[dospunk]

@sand4rt While the events/emits pattern is typical for Vue, it also does allow taking functions as props. I prefer to use functions as props because I find that it gives me more control through typing with typescript. You can only provide types for events/emits in Vue if you're using the <script setup> syntactic sugar. While it may not be the established best practice, I feel that a complete testing library for Vue should support it because Vue itself supports it. I can open an issue for further discussion if you'd like!

As for more complex objects, I mean objects that may contain functions. For example, an instance of a class.

[unitydynamics]

I'd like to add my voice to reverse this change. You'll find that people cannot move from 1.31 to 1.32 because of this change specifically. I don't think it had the desired effect in terms of making code "safer"--this new restriction does not match what Vue considers good props. You really should let Vue decide what a bad prop is (and leave the type to unknown). If you need examples, I can show how this change essentially broke most Typescript code that uses props in their Vue components.

[sand4rt]

@dospunk yeah sure, there are similar issues but i think it would be good to keep track of that!

@unitydynamics could you provide an example that worked before the change and doesn't work after the change?

[unitydynamics]

@sand4rt The primary issue (at least for me) is that I define all of my interesting props with Typescript interfaces, and the JsonObject type doesn't allow for it... This isn't a full blown component test, but you can get the idea here.

[sand4rt]

@unitydynamics the example seem to work as expected, do I miss something?:

interface MyType {
  test: string;
}
const myTestProp = { test: 'hello' }

type JsonPrimitive = string | number | boolean | null;
type JsonValue = JsonPrimitive | JsonObject | JsonArray;
type JsonArray = JsonValue[];
type JsonObject = { [Key in string]?: JsonValue };

const props: JsonObject = {
  primitiveStringProp: 'works fine',
  objectProp: { test: 'also works fine', test2: 4 }, // there was a missing comma here
  interfaceProp: myTestProp, // doesn't type check!
}

[dospunk]

@sand4rt you're missing the type definition on myTestProp, line 4 should be

const myTestProp: MyType = { test: 'hello' }

[unitydynamics]

You've simply removed Typescript from the relevant code.
What I have in my code:
const myTestProp:MyType = {
is significantly different than what you have in your code:
const myTestProp = {

In your code myTestProp could be assigned to anything. In my code, Typescript will help me ensure that I've created my test prop appropriately. This is critical to writing and maintaining good tests. Developers usually want durable tests, and it's nice when we can lean on Typescript to ensure their test props conform to the expected type.

So in my opinion, the 1.32 release didn't add anything to type safety of testing--but it made it much worse!

[sand4rt]

@unitydynamics
Ah i see, accidentally remove that part. Hadn't thought of this before.. I usually use type instead of interface. With type you don't have this problem: https://www.youtube.com/watch?v=zM9UPcIyyhQ.

You need to extend JsonObject if interface is used.

Not sure if we should force type instead of interface or extend the JsonObject.. @pavelfeldman what do you think, does this change needs to be reverted?

@dospunk The problem of non-json serialisable data as props remains. This is a known issue, see related issues: #22003 (comment).

[unitydynamics]

Thanks for your consideration. I think understand why the props need to be Json serializable. For my part, I think throwing a runtime error is completely appropriate if a prop cannot be serialized. And personally I'm OK with that restriction (I don't need functions or vue refs or other non-data props)

But please don't prohibit the use of Typescript interfaces! That would prevent most modern Vue developers from using playwright ct.

[sand4rt]

@unitydynamics @dospunk thanks for sharing your thoughts! I've made a PR to revert the change. Let's wait for the Playwright team to respond: #22005.