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

feat(ct): vue type safe mount props #23151

Merged
merged 2 commits into from
Aug 1, 2023

Conversation

sand4rt
Copy link
Collaborator

@sand4rt sand4rt commented May 18, 2023

partial fix for: #17206

https://www.npmjs.com/package/vue-component-type-helpers has been released last month and is maintained by Vue. It's also used in Vue Test Utils: https://github.com/vuejs/test-utils/blob/main/package.json#L26

related: #23194

@sand4rt sand4rt force-pushed the ct-vue3-typesafe-props branch 3 times, most recently from 5e8a5b3 to f382eb9 Compare May 21, 2023 12:14
component: any,
options: MountOptions<HooksConfig, never> & { props: Props }
): Promise<MountResult<Props>>;
mount<HooksConfig extends JsonObject, Component = unknown>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think this will break existing clients? or do we want to break them with these stricter types?

Copy link
Collaborator Author

@sand4rt sand4rt May 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you think this will break existing clients?

A subset of existing clients using the Vue object API could see some type errors:

  1. When the prop is manually passed to the mount<_, Props>() generic.

  2. When a required prop is missing or when a wrong prop type passed to the mount().

  3. When a invalid prop or non existing type is passed to component.update()

do we want to break them with these stricter types?

IMO yes. It is cumbersome to specify prop types manually.

When a wrong prop type is passed I think it would be good to know that immediately instead of running the test and checking if Vue logged anything to the console.

I think it's also useful to have autocompletion on both mount() and component.update().

@sand4rt
Copy link
Collaborator Author

sand4rt commented May 24, 2023

related to: testing-library/vue-testing-library#305

@pavelfeldman
Copy link
Member

👋 @sand4rt what should we do with this one?

@sand4rt sand4rt force-pushed the ct-vue3-typesafe-props branch from f382eb9 to 5e1720d Compare July 29, 2023 19:44
@github-actions
Copy link
Contributor

Test results for "tests 1"

2 interrupted
⚠️ [playwright-test-windows-latest-node16] › reporter-html.spec.ts:1632:7 › created › labels › if label contains similar words only one label should be selected
⚠️ [playwright-test-windows-latest-node16] › reporter-html.spec.ts:1710:7 › created › labels › handling of meta or ctrl key

62 passed, 524 skipped
✔️✔️✔️

Merge workflow run.

@github-actions
Copy link
Contributor

Test results for "tests 1"

1 failed
❌ [playwright-test-macos-latest-node16] › reporter-html.spec.ts:1452:7 › created › labels › filter should update stats

10 flaky
⚠️ [chromium-ubuntu-22.04-node20] › page/page-event-request.spec.ts:129:3 › should report navigation requests and responses handled by service worker with routing
⚠️ [chromium-ubuntu-20.04-chromium-tip-of-tree] › library/tracing.spec.ts:239:5 › should not include trace resources from the previous chunks
⚠️ [chromium-ubuntu-20.04-chromium-tip-of-tree] › page/page-event-request.spec.ts💯3 › should report navigation requests and responses handled by service worker
⚠️ [webkit-ubuntu-22.04-node16] › library/browsercontext-reuse.spec.ts:50:1 › should reset serviceworker
⚠️ [webkit-ubuntu-22.04-node16] › library/inspector/cli-codegen-pytest.spec.ts:56:5 › should save the codegen output to a file if specified
⚠️ [playwright-test-macos-latest-node16] › ui-mode-test-progress.spec.ts:167:5 › should update tracing network live
⚠️ [playwright-test-macos-latest-node16] › ui-mode-trace.spec.ts:22:5 › should merge trace events
⚠️ [playwright-test-macos-latest-node16] › ui-mode-trace.spec.ts:126:5 › should show snapshots for sync assertions
⚠️ [playwright-test-windows-latest-node16] › ui-mode-test-progress.spec.ts:22:5 › should update trace live
⚠️ [playwright-test-windows-latest-node16] › ui-mode-trace.spec.ts:22:5 › should merge trace events

25022 passed, 583 skipped
✔️✔️✔️

[playwright-test-macos-latest-node16] › reporter-html.spec.ts:1452:7 › created › labels › filter should update stats

Error: expect(received).toBe(expected) // Object.is equality

Expected: "Total time: 1424ms"
Received: "Total time: 1.4s"

  1774 |         await expect(page.locator('.chip', { hasText: 'c.test.js' })).toHaveCount(1);
  1775 |
> 1776 |         await flakyButton.click();
       |                                 ^
  1777 |
  1778 |         await expect(searchInput).toHaveValue('@regression @flaky');
  1779 |         await expect(page).toHaveURL(/@regression%20@flaky/);

    at checkTotalDuration (/Users/runner/work/playwright/playwright/tests/playwright-test/reporter-html.spec.ts:1776:33)
    at /Users/runner/work/playwright/playwright/tests/playwright-test/reporter-html.spec.ts:1782:9

Merge workflow run.

@sand4rt
Copy link
Collaborator Author

sand4rt commented Jul 29, 2023

👋 @sand4rt what should we do with this one?

I resolved the merge conflicts and comment. Would really like to see this merged. @vue/test-utils decided to do this as well and playwright-ct-svelte also has typed props.

@sand4rt sand4rt requested a review from pavelfeldman July 29, 2023 21:41
@sand4rt sand4rt force-pushed the ct-vue3-typesafe-props branch from 4b915b0 to 3f42ed6 Compare July 30, 2023 08:45
@github-actions
Copy link
Contributor

Test results for "tests 1"

6 flaky
⚠️ [chromium-ubuntu-22.04-node20] › library/tracing.spec.ts:239:5 › should not include trace resources from the previous chunks
⚠️ [chromium-ubuntu-22.04-node20] › page/page-event-request.spec.ts:129:3 › should report navigation requests and responses handled by service worker with routing
⚠️ [webkit-ubuntu-22.04-node16] › library/browsercontext-reuse.spec.ts:50:1 › should reset serviceworker
⚠️ [chromium-ubuntu-22.04-node16] › library/tracing.spec.ts:239:5 › should not include trace resources from the previous chunks
⚠️ [firefox-ubuntu-22.04-node16] › page/frame-goto.spec.ts:46:3 › should continue after client redirect
⚠️ [playwright-test-macos-latest-node16] › ui-mode-trace.spec.ts:53:5 › should merge web assertion events

25027 passed, 583 skipped
✔️✔️✔️

Merge workflow run.

@pavelfeldman pavelfeldman merged commit 7c5d73a into microsoft:main Aug 1, 2023
Germandrummer92 pushed a commit to OctoMind-dev/playwright that referenced this pull request Oct 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants