chore: restructure ByTestId queries code by predicate type

[MattAgn]

Summary

This PR aims to solve this issue.

Tasks:

• group queries by their predicate type (byTestId, byText) instead of grouping them by their return
  type (different for get, find, query).
• make query builders that use the queryAllBy query as the base to create the getBy, getAllBy, queryBy, findBy and findAllBy queries
• group the tests by predicate type as well

Requirements

• The API should not change

Test plan

No tests should be added since the API does not change, they only need to be moved to be grouped by predicate type

[MattAgn]

@thymikee @mdjastrzebski here's what I did so far:

• make new query builders based on queryAll
• use them to generate byTestId queries
• group related tests

Apart from that, I'm not too happy about the typing of the query builders I implemented (there is an any that I don't know how to replace with flow).

Is this kind of refactor what you had in mind?

[thymikee]

Looks promising! Nice job

[thymikee]

@MattAgn I've pushed to your fork with updated typings and rebase after the upgrade to latest RN with newer Flow version. Can you take a look? :)

[MattAgn]

Thanks @thymikee! It's all good to me, thanks for the help on the typing!

For the error messages, I wonder if it would be a good idea to generify them as well to keep them coherent across the different queries. Like "No instances found with {ATTRIBUTE} = "myAttribute". From what I've seen, the message is already identical (except for the attribute) for most queries right?

[thymikee]

I've changed the error messages here to reflect what's in the DOM testing library, as I find the wording nicer. We can copy this 1:1, no problem from our side. We can also change it any time and surely make generic where it makes sense

[mdjastrzebski]

Overall looks going into right direction. I'm glad you started working on this improvement 🚀

I would suggest looking into src/helpers/makeQuery.js which has similar functionality of building different query types from queryAll query. That code has a little more functions, as it's also responsible for building query aliases which is not relevant for this PR.

IMO best API would be to have a single makeQueries function taking in queryAllByXxx query (and maybe some more args) and returning object with all query types: {query,get,find}[All]ByXxx. With the current code you will have to repeat the query construction (as in byTestId.js lines 27-55) for every query type. Finally we could export the results with proper names and typing: getBy => getByTestId

[MattAgn]

Thanks for the review @mdjastrzebski! I'll take care of the requested changes

[thymikee]

@MattAgn I made Queries parametrized with QueryArg, should solve the problem of queries having different args.

[MattAgn]

Thanks @thymikee! I tried it as well but couldn't make it work with the spread operator if we were to add options to the queries. Should we ignore the options for now and tackle the issue once the options are done?

[thymikee]

Exciting!

[thymikee]

@mdjastrzebski ping :)

[mdjastrzebski]

Excellent work! This code is well-structured and easy to read. 🥇🥇

I guess that as a follow-up PR(s) we could work reusing makeQueries to generate all other queries.

PS. Really sorry for long waiting time

[mdjastrzebski]

Consider adding explicit types

[thymikee]

We can address that in a followup if necessary (I suspect Flow infers that, as it's internal fn)

[MattAgn]

I confirm, flow infers it

[MattAgn]

@thymikee @mdjastrzebski thanks for the approval! (it was my first open source contribution and it's been a great experience!)
For the following work on makeQueries, would you rather have one PR with all queries or a bunch of PRs with a few queries addressed in each?

[mdjastrzebski]

@MattAgn Now we have the common part of the code already solved, as well as pattern to follow. IMO I would go with one PR, as it boils down to defining queryAll for each query type and proper exports. Tests and TS typedefs probably don't need to change.

@thymikee wdyt?

[thymikee]

IMO I would go with one PR, as it boils down to defining queryAll for each query type and proper exports. Tests and TS typedefs probably don't need to change.

👍🏼. We can, however, rearrange tests later to comply to the new structure, if needed :)