Reorganize internal queries code by predicate typ

[mdjastrzebski]

Describe the Issue

Current queries are organized in files by return type (getByAPI.js, queryByAPI.js, findByAPI.js) with exception for A11yAPI.js which is organized by predicate (byA11yLabel, byA11yHint, etc). Generally getBy and getAllBy queries are used as core query, with queryBy/queryAllBy and findBy/findAllBy as manually written wrappers around getBy/getAllBy quries.

There is a lot of repeated code, especially in duplicate getBy and getAllBy queries as well as manual queryBy/queryAllBy/findBy/... wrappers.

This presents an opportunity to refactor queries so that for each predicate type (ByText, byA11yLabel, etc) there is only function encoding the search algorithm, while all other functions are generated by some generic code.

In Testing Library this is achieved by implementing query predicates as queryAll function first, and then using generic buildQueries function that returnes getBy/getAllBy/etc variants.

Benefits

1. Single definition of each predicate, so avoiding recent getByTestId/getAllByTestId differences.
2. Easier creation of additional predicates, you just need to create queryAll version and all other queries will generated for you by buildQueries function.
3. Reduced code side

Downsides

None found yet.

Considerations

1. This change should not change the API
2. We should keep the same unit tests, but we might reorganize them by predicate vs by return type to match new queries file structure.
3. Queries performance should be the same, because react-test-renderer find uses findAll under the hood without any query optimization.
4. Typescript typings tests should probably stay the same, i.e. repeated use-case statements, and not be generated for code correctness reasons.

[thymikee]

Good call, let's do it.

3. Queries performance should be the same, because react-test-renderer find uses findAll under the hood without any query optimization.

This is an implementation detail. I guess that in some far-away future some optimizations may be added, but it's not the case now, and I don't think it has any priority. Once RTR does that, we can think about including these optimizations.

[MattAgn]

I'd be glad to lend a hand on this issue @thymikee @mdjastrzebski. I could try to do it for one query before validating with you, what do you think?

[thymikee]

@MattAgn lovely!

[mikeduminy]

It looks like the PRs above have changed the results of the API. Before these changes the getBy functions would be:

try {
  return instance.find((node) => getNodeByText(node, text));
} catch (error) {
  throw new ErrorWithStack(
    prepareErrorMessage(error, 'text', text),
    getByTextFn
  );
}

I assume this returns the first result that satisfies the condition.

Compare this to the new getBy function:

const results = queryAllByQuery(instance)(args);

if (results.length > 1) {
  throw new ErrorWithStack(getMultipleError(args), getFn);
}

if (results.length === 0) {
  throw new ErrorWithStack(getMissingError(args), getFn);
}

return results[0];

In this case, the API contract is different since now an error is thrown if the query finds more than 1 result. This might be an improvement since it is clearer and more aligned with dom-testing-lib but it is different.

In the project I'm working on this forces us to re-write our queries from:

// FROM
const { getByText } = render(<MyComponent />)
await fireEvent.press(getByText('.cancel'))

// TO
const { getAllByText } = render(<MyComponent />)
await fireEvent.press(getAllByText('.cancel')[0])

Granted this is because we have a design system that results in multiple elements being detected (a button element + a label for the button = 2 results). This is just in case anyone else comes across this.

[mikeduminy]

Queries performance should be the same, because react-test-renderer find uses findAll under the hood without any query optimization.

I don't think this is correct - find calls findAll(predicate, {deep: false}) which doesn't recursively search through children (source).

If this search is synchronous this might explain why on super deep render trees (like those used in the app in which I work) some tests can take 15s to complete, even if the test timeout is 5s.

[mdjastrzebski]

Seems like the transition to by predicate code organisation is mostly done:

• common query builder (makeQueries) in order to define predicate once
• put each query predicate in separate file instead of grouping queries by query verb (get, query, ...)
• tests grouped by predicate instead of query verb

To finish the transition I think we should do some final smaller tasks:

• Replace GetByAPI, QueryByAPI and FindByAPI types and functions (xxxByAPI) with per query type (ByXxx) interfaces
• Migrate A11y query builder (makeA11Query) to use standard query builder(makeQueries) in order to allow more flexibity with query parameters and also improve code coherence, reduce over-abstraction.
• Move query implementations from src/helpers to src/queries

@thymikee @MattAgn @AugustinLF wdyt?

[thymikee]

@mdjastrzebski 👍🏼

[MattAgn]

Yes sounds good! I can help on the A11y queries if you want

[mdjastrzebski]

@MattAgn could you do PR review for #965 first?

[MattAgn]

Sure on it!

[thymikee]

#965 is merged. Anyone willing to port a11y queries? :)

[mdjastrzebski]

@MattAgn you wanted to volunteer so let me know if you want to pick it up :-) otherwise I can work on it.

Couple of thoughts on that topic:

1. makeA11yQuery.ts has too much abstractions inside, so it's not a good model for code that is easy to reason.
2. Each a11y query should be put into separate file in src/queries folder, e.g. src/queries/labelText.ts
3. The file should resemble current testId.ts structure. So that we first have queryAllByXxx predicate with accompanying getMultipleError and getMissingError which we pass to makeQueries to get derived queries (getBy, etc), and finally we export ByTestIdQueries type bindByTestIdQueries function.
4. That means that from the existing a11y code we are only interested in extracting logic for queryAllByXxx predicates.
5. Aliases should be made by hand, as it is easier to understand:

export const bindByLabelTextQueries = (
  instance: ReactTestInstance
): ByLabelTextQueries => {
  const boundGetBy = getBy(instance),
  //...

  return {
    getByLabelText: boundGetBy,
    getByA11yHint: boundGetBy,
  // ...
  }
}

Note: that boundGetBy is used to have the same function instance to be assigned for all relevant aliases.

Little repetition hurt nobody, unlike too much abstraction.

[MattAgn]

@mdjastrzebski yes i'm on board with all your points
is it alright if I tackle that next week?

[mdjastrzebski]

@MattAgn that works for me 👍🏻

[MattAgn]

@mdjastrzebski should we close this issue now that our PR is merged?