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

Adding support for extending with custom queries #573

Closed
wants to merge 6 commits into from

Conversation

pvinis
Copy link

@pvinis pvinis commented Oct 9, 2020

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

@mikeduminy
Copy link
Contributor

This would be a huge time saver while we wait for other PRs to be merged. +1

@klekowskim
Copy link
Contributor

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

@pvinis
Copy link
Author

pvinis commented Oct 10, 2020

I can rename it 👍
I haven't used that lib so I'm not familiar. I'll look into it a bit.

@mdjastrzebski
Copy link
Member

mdjastrzebski commented Oct 12, 2020

I like the idea of allowing custom queries in RNTL.

Big heads up to using the same API as React Testing Library because:

  • we re-use RTLs effort that went into creating that API & avoid potential mistakes
  • we allow devs to easily transition between web/RN tests
  • we allow some code reusage between web/RN

RTL's also exposes buildQueries so that devs can easily add new quieries in all variants (getBy, getAllBy, queryBy,...) by providing just queryAllBy variant. It would be good to provide that as well.

@thymikee
Copy link
Member

thymikee commented Oct 12, 2020

RTL's also exposes buildQueries so that devs can easily add new quieries in all variants (getBy, getAllBy, queryBy,...) by providing just queryAllBy variant. It would be good to provide that as well.

Sounds like a good idea for another contribution, that would align with some refactoring we plan: #325

@pvinis
Copy link
Author

pvinis commented Oct 19, 2020

I renamed it. Is this good to go then? And after this is merged we can work on the buildQueries with the refactoring anyway.

@thymikee
Copy link
Member

Can you make sure lint and flow pass?

@pvinis
Copy link
Author

pvinis commented Oct 19, 2020

Yup yup. I thought the last commit would fix that 😬.

@pvinis
Copy link
Author

pvinis commented Oct 19, 2020

Interestingly it complains even though I have an ignore right above.
https://github.com/callstack/react-native-testing-library/pull/573/files#diff-9c1abad9b45f5a8bc3d32761977018ab6ede546246de46f8c7b1cbf75b320651R384

That instance there is needed, since that's how custom queries will access the instance. I just made one that is not using it for the test.

Copy link
Member

@thymikee thymikee left a 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,
Copy link
Member

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

Copy link
Member

@mdjastrzebski mdjastrzebski left a 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.

src/__tests__/render.test.js Outdated Show resolved Hide resolved
@@ -293,6 +296,9 @@ export interface Thenable {
export interface RenderOptions {
Copy link
Member

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;
}

Source: https://github.com/testing-library/dom-testing-library/blob/master/types/get-queries-for-element.d.ts

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.

Copy link
Member

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 <mdjastrzebski@gmail.com>
@thymikee
Copy link
Member

@pvinis we just released v7.1.0 with container exposed from render, so queries is no longer necessary for your use case. We can wait a bit more with merging this unless we have a good type story around that for Flow and TS.

@mdjastrzebski mdjastrzebski added help wanted Extra attention is needed good first issue Good for newcomers labels May 4, 2022
@mdjastrzebski
Copy link
Member

Closing the PR as a stale code, however the idea is worth pursuing so I've created an issue #971 based on this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants