Proposal: Alternative API with chain-able queries

[kevin940726]

Describe the feature you'd like:

The original motivation of this proposal is coming from the performance perspective. *ByRole queries are quite slow, compared to the others, and especially in the jsdom environment. There are already several issues of this: #698, #820. The solutions proposed there were usually either passing hidden: true to the options, or using another query. Both are not ideal and we would be losing some confidence and specificity to our tests.

Needless to say, the ultimate solution will always be to improve the performance of the *ByRole queries directly. However, that seems to be unlikely to happen any time soon. This got me thinking that if there are any alternatives.

The basic idea of this proposal is to allow queries to be chained together so that multiple queries together can increase the specificity. Take the below code for instance:

const button = screen.getByRole('button', { name: /click me/I });

On jsdom, with large DOM tree, this query would sometimes take seconds to evaluate. If we change it to use getByText, however, would usually take less than 100ms:

const button = screen.getByText(/click me/i);

These two are not interchangeable though. We want the performance of getByText, but we also want the specificity getByRoles provides. What if we can combine those two queries?

// Imaginary API
const button = screen.getByTextThenByRole(/click me/i, "button");

Suggested implementation:

The proposal is to split queries into two parts: queries and filters.

Queries are high-level selector function like get, getAll, find, etc. They take a container and a list of filters as arguments and return the result element.

Filters are pure higher-order functions which take node element as argument and determine if the node matches given creteria. For instance, byText(/click me/i)(node) checks if node has text content of /click me/i and returns true or false.

Importing:

import {
  // queries
  get,
  getAll,
  query,
  queryAll,
  find,
  findAll,

  // filters
  byText,
  byTestId,
  byRole
} from "@testing-library/dom";

Queries:

// Get an element with a filter
const button = get(container, byText(/click me/i));

// Find an element with a filter
const button = await find(container, byText(/click me/i));

// Get an element with multiple filters
const button = get(container, byText(/click me/i), byRole("button"));

// Get multiple elements with multiple filters and optional options
const buttons = getAll(
  container,
  byText(/click me/i, {
    exact: false
  }),
  byRole("button")
);

With screen. Auto-bind the first argument to the top-most container.

const button = screen.get(byText(/click me/i));

const buttons = await screen.findAll(byText(/click me/i), byRole("button"));

Here are some of the highlights and trade-offs in this proposal:

Performance

As mentioned earlier in the motivation, the *ByRole queries are quite slow. We can now rewrite that query into something like this.

const button = screen.get(byText(/click me/i), byRole("button"));

What it says is: First we get all the elements that have text content of /click me/i, and then we filter them by their roles and only select those with has a role of button. Finally, we return the first element of the list or throw an error if the number of the list is not one.

Note that this is still not interchangeable with the getByRole query earlier, but a closer alternative than getByText alone.

The order of filters matters, if we filter byRole first, then it's gonna be just time-consuming or even worse as in our original problem.

Specificity

Chain-able queries can increase specificity level by combining multiple queries. As shown in the above example, byText + byRole has higher specificity than getByText only, and also achieves a similar effect as in getByRole but with performance benefits.

However, we could argue that combining multiple queries is rarely needed. The queries we provide are already very specified. We don't normally need to chain queries together unless for performance concerns. Perhaps we could provide a more diverse set of filters to be chained together with specificity in mind.

// Instead of
screen.geyByLabelText("Username", { selector: "input" });
// We could do
screen.get(byLabelText("Username"), bySelector("input"));

By introducing an imaginary bySelector filter, we can now split byLabelText with options into two different filters. The bySelector filter can also be reused in combination with other filters like byText as well. Some other examples below:

// From
screen.getByRole("tab", { selected: true });
// To
screen.get(byRole("tab"), bySelected(true));

// From
screen.getByRole("heading", { level: 2 });
// To
screen.get(byRole("heading"), byLevel(2));

Whether these filters have the correct abstractions is not the main point of this proposal, but only to serve as an inspiration for the potential work we could do.

Extensibility

Filters are just pure higher-order functions. We can create our own filter functions with ease. Below is a simplified version of queryAllByDataCy mentioned in the Add custom queries section.

function byDataCy(dataCyValue) {
  return function(node) {
    return node.dataset.cy === dataCyValue;
  };
}

screen.get(byDataCy("my-cy-id"));

What is unknown is how we should handle the error messages. A potential solution would be to bind getMultipleError and getMissingError to the function and let get handles the rest.

byDataCy.getMultipleError = (c, dataCyValue) =>
  `Found multiple elements with the data-cy attribute of: ${dataCyValue}`;
byDataCy.getMissingError = (c, dataCyValue) =>
  `Unable to find an element with the data-cy attribute of: ${dataCyValue}`;

Another solution would be to return an object in the filter function, like have seen in Jest's custom matchers API.

function byDataCy(dataCyValue) {
  return function(node) {
    return {
      pass: node.dataset.cy === dataCyValue,
      getMultipleError: (c, dataCyValue) =>
        `Found multiple elements with the data-cy attribute of: ${dataCyValue}`,
      getMissingError: (c, dataCyValue) =>
        `Unable to find an element with the data-cy attribute of: ${dataCyValue}`
    };
  };
}

The final API is TBD, but I would prefer the latter since it's more flexible.

Not only are the filters extensible, but we could also create our own queries variants as well, though unlikely to be useful. Let's say we want to create a new query called includes, which essentially is the same as queryAllBy* but returns boolean if there's a match.

function includes(container, ...filters) {
  const nodes = queryAll(container, ...filters);
  return nodes.length > 0;
}

includes(container, byText(/click me/i)) === true;

Direct mapping to Jest custom matchers

@testing-library/jest-dom is a really helpful tool. It almost has 1-to-1 mappings between @testing-library/dom and Jest, but still lacking some matchers.

With this proposal, we can create custom matchers very easily with direct mappings to all of our existing filters without duplicate code.

expect.extend({
  toMatchFilters(node, ...filters) {
    const pass = filters.every(filter => filter(node));
    return {
      pass,
      message: () => "Oops!"
    };
  }
});

expect(button).toMatchFilters(byRole("button"), byText(/click me/i));

within for complex queries

Let's say we want to select the button under the my-section section, we can query it like this:

const mySection = getByTestId("my-section");
const button = within(mySection).getByRole("button");

What if we can query it in one-go with an imaginary API?

const button = get(
  byTestId("my-section"),
  within(), // or dive(), getDescendents(), getChildren(), ...
  byRole("button")
);

Not sure how helpful would that be though, just a random thought regarding this proposal.

Describe alternatives you've considered:

One drawback of this proposal is that we can no longer import every query directly from screen, we have to also import byText, byRole, byTestId, etc. However, I believe modern editors should be able to auto-import them as soon as we type by* in the filters.

If that's unavailable, there's a not-so-great alternative proposal in chain-able style:

const button = screen
  .byText(/click me/i)
  .byRole("button")
  .get();

const buttons = await screen
  .byTest(/click me/i)
  .byRole("button")
  .findAll();

In this alternative proposal, we have to also provide a screen.extend API to be able to use custom queries though.

Teachability, Documentation, Adoption, Migration Strategy:

The drawback of this proposal is obvious: we don't want to introduce new APIs to do the same things, especially when the current behavior already satisfies 80% of the usages. I can totally understand if we decide to not go with this approach. We also don't want to deprecate the current API any time soon. Fortunately, though, this proposal can be fully backward-compatible. We can even provide a codemod if we like to encourage this usage.

IMHO, I think we should recommend the new API whenever possible, as they cover all the cases of the old API but also open to more opportunities. We could ship these new API as a minor version first, and then start deprecating the old API in a few major releases (but still supporting them). That is, of course, if we are happy about this proposal.

WDYT? Not worth the effort? Sounds interesting? Appreciate any kind of feedbacks :).

[marcosvega91]

Hi @kevin940726 thanks for reaching us, new ideas on improvement are always welcome 😄

What you have described seems very interesting. Introducing this kind of implementation could add great flexibility and modularity, but what are the benefits? We need to understand what will be the advantage of this approach in terms of performance.

An idea could be to create a small example for a medium/big size DOM comparing the actual solution with your's one.

Let's wait for the opinions of other maintainers.

[gnapse]

Thanks for taking the time to draft that proposal. It certainly looks interesting.

Regardless of how open we maintainers may be with this proposal, I am 99.99% sure it is something that would first need to be incubated in a separate package to be able to judge more thoroughly its real convenience. And even then, it has to justify making a huge breaking change if we were to adopt it.

The need to importe multiple stuff is also IMO a downside in ergonomics. Yes, modern editors auto-import, but still. Lots of names when today a single screen import gives you everything.

Anyway, I am not against it, but a sample implementation would go a long way into being able to better judge this.

[kevin940726]

An idea could be to create a small example for a medium/big size DOM comparing the actual solution with your's one.

Here's an ad-hoc minimal implementation on CodeSandbox. Need to download it as zip and run it in a local environment though.

The result from my device (macOS 2.3 GHz i7, Node v14.15.1) is as follow:

screen.getByTestId: 29 ms
screen.getByText: 58 ms
screen.getByRole: 3549 ms
screen.get(byText(), byRole()): 450 ms
screen.get(byRole(), byText()): 6379 ms

Note that the ad-hoc implementation uses cloneNode which should be slower than actual implementation, but we can still see a performance boost from screen.getByRole to screen.get(byText(), byRole()) (3549 -> 450). The inverse though (screen.get(byRole(), byText())) is a lot slower since it has to run 2 different filters (and also cloneNode).

[kevin940726]

I am 99.99% sure it is something that would first need to be incubated in a separate package to be able to judge more thoroughly its real convenience. And even then, it has to justify making a huge breaking change if we were to adopt it.

Does that mean it would be better for discussion if I submit a draft PR with a POC?

I don't think that this would be a breaking change though, maybe we have a different mindset about what a breaking change is 🤔?

The need to importe multiple stuff is also IMO a downside in ergonomics. Yes, modern editors auto-import, but still. Lots of names when today a single screen import gives you everything.

100% agree. This is the main issue of this proposal (besides from increasing the number of API), but I think it would be worth it. Maybe, I hope so, IDK 😂.

[gnapse]

I don't think that this would be a breaking change

If what you mean is that we can still provide the exports of the current queries, then yes. But what's the benefit of having two alternative ways to query the DOM at the same time? It can create confusion in users of this library.

But yes, technically it does not need to be a breaking change.

[alexkrolick]

Previously:

• Separation of concerns in getByX / queryByX / etc. API #266 RFC for fetchBy* queries #769 within for mutliple elements #420 getClosestBy* #327 Support querying arrays of elements #294
• https://testing-library.com/docs/ecosystem-testing-library-selector
• https://testing-library.com/docs/ecosystem-rtl-simple-queries
• https://testing-library.com/docs/ecosystem-query-extensions

[gnapse]

I knew I felt a deja-vu when reading this proposal. Thanks @alexkrolick!

[kevin940726]

Oops! I did search for prior arts but couldn't find those! Thanks for the links.

Do we still stand by the decision made by @kentcdodds in #266, or does the potential performance benefits here outweigh it? Feel free to close this if we decide to not go with this path!

[nickserv]

I like the idea of having a more composable API, but I don't think users would use a more complex API just to improve the performance of one query. It also requires some knowledge of which queries are faster. Perhaps there's another way we can compose these that memoizes common lookups in a way that doesn't require this knowledge.

[timkindberg]

We are dealing with tons of performance issues due to what we believe (with a handful of anecdotal instances) to be from getByRole usage. I really like the idea of adding the ability to choose by multiple things in a particular order to improve performance.