-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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): react optional declaration merging hooks config #31055
base: main
Are you sure you want to change the base?
feat(ct): react optional declaration merging hooks config #31055
Conversation
sand4rt
commented
May 28, 2024
- Partial fix for: [Feature] improve type safety hooksConfig #19933
- @pavelfeldman this is a split from: feat(ct): resolve hooksConfig import refs #30455 (comment)
- Will follow up with the other frameworks if this one gets through
This comment has been minimized.
This comment has been minimized.
route?: string; | ||
routing?: boolean; | ||
declare module '@playwright/experimental-ct-react/hooks' { | ||
interface RegisterHooksConfig { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer to not override types globally in Playwright. Can we help the user in some other way? Perhaps by casting test
exported from the package like const test = baseTest as SomeTestDeclaration<SpecificHooksConfig>
? Or by extending with a mnt
fixture that is mount<SpecificHooksConfig>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We prefer to not override types globally in Playwright.
Normally i wouldn't use declare module
in this way, but i think this is a good use case. Especially because the mount()
is called so many times. The beforeMount<T>
, afterMount<T>()
and mount<T>()
can also still be specified manually for the people who think it's too much magic.
Can we help the user in some other way?
I don't think there's another way to make the hooksConfig
in beforeMount()
, afterMount()
and mount()
automatically type-safe with a single type definition right? type-casting const test
would solve a part of the problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's another way to make the
hooksConfig
inbeforeMount()
,afterMount()
andmount()
automatically type-safe with a single type definition right? type-castingconst test
would solve a part of the problem.
That's right. However, you only write beforeMount()
and afterMount()
once, so explicitly typing them is not a big deal. It looks like the main goal is to avoid mount<T>
every time.
d1447ef
to
a43d5d6
Compare
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"15 passed Merge workflow run. |