-
Notifications
You must be signed in to change notification settings - Fork 273
Adding support for extending with custom queries #573
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
Conversation
This would be a huge time saver while we wait for other PRs to be merged. +1 |
I personally would go with the same API and behaviour as in react-testing-library: https://github.com/testing-library/react-testing-library/blob/c546a6f4927f925bf1187e631ca0444001f067f5/src/pure.js#L35 |
I can rename it 👍 |
I like the idea of allowing custom queries in RNTL. Big heads up to using the same API as React Testing Library because:
RTL's also exposes |
Sounds like a good idea for another contribution, that would align with some refactoring we plan: #325 |
I renamed it. Is this good to go then? And after this is merged we can work on the buildQueries with the refactoring anyway. |
Can you make sure lint and flow pass? |
Yup yup. I thought the last commit would fix that 😬. |
Interestingly it complains even though I have an ignore right above. That |
bc092ab
to
505963a
Compare
505963a
to
3a2a2b1
Compare
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.
Looks good! I made some final adjustments for the API to be compatible with Testing Library
...findByAPI(instance), | ||
...a11yAPI(instance), | ||
...(queries: { | ||
[key: $Keys<typeof queries>]: (...rest: Array<any>) => any, |
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.
Struggling a bit on how to type it better, so that custom queries are inferred without forced generics. We can improve it later though
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.
Overall looks good, consider adding generic typedefs. I've provided source for RTL/DTL ts types, so we should be able to easily apply them. Flow might be more difficult but I guess that typescript is now more popular in the ecosystem.
Anyway, we still need TS types tests + some docs.
@@ -293,6 +296,9 @@ export interface Thenable { | |||
export interface RenderOptions { |
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 check RTL/DTL and they seem to have quite decent TS typings for custom queries:
export type Query = (
container: HTMLElement,
...args: any[]
) => Error | Promise<HTMLElement[]> | Promise<HTMLElement> | HTMLElement[] | HTMLElement | null;
export interface Queries {
[T: string]: Query;
}
export type RenderResult<Q extends Queries = typeof queries> = {
// ...
} & {[P in keyof Q]: BoundFunction<Q[P]>}
export interface RenderOptions<Q extends Queries = typeof queries> {
// ...
queries?: Q
}
Source: https://github.com/testing-library/react-testing-library/blob/master/types/index.d.ts
IMO we could have the same at least for TS, possibly for flow as well if possible.
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.
Please note that the Query
type restricts return value to (Promise)(Array) Element + null + Error. IMO if we provide element queries then we should also consider similar restriction. wdyt?
Co-authored-by: Maciej Jastrzebski <[email protected]>
Closing the PR as a stale code, however the idea is worth pursuing so I've created an issue #971 based on this PR. |
Summary
After the discussion on #569 and #566, I tried to make the prototype for this.
I think it's kind of alright, but I need some help with the flowtyping, as I'm not too familiar with it.
Test plan