Skip to content

Support querying arrays of elements #294

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

Closed
sarenji opened this issue Jun 21, 2019 · 2 comments
Closed

Support querying arrays of elements #294

sarenji opened this issue Jun 21, 2019 · 2 comments

Comments

@sarenji
Copy link
Contributor

sarenji commented Jun 21, 2019

Describe the feature you'd like:

I wanted to propose querying an array of containers rather than a single container. It would allow us to do queries like this:

getByText(getAllByRole('heading'), 'Page 2'));

... which is more composable. Currently you can only query one container at a time, not an array of them.

As for a semi-practical example, let's say I'm on a page with the following (convoluted) JSX:

const pages = ['Page 1', 'Page 2'];

const App = () => {
  const [title, setTitle] = useState(pages[0]);
  return (
    <main>
      <h1>{title}</h1>
      <nav aria-labelledby="inner">
        <h2 id="inner">Inner nav</h2>
        <ul>
          {pages.map((pageTitle) => (
            <li>
              <a href="#url" onClick={() => setTitle(pageTitle)}>
                {pageTitle}
              </a>
            </li>
          ))}
        </ul>
      </nav>
    </main>
  );
};

I want to check that clicking Page 2 results in the page heading changing from Page 1 to Page 2.

However, since the same text is inside the heading and the nav (and potentially more in a real app), it's hard to select the proper element. With the new API, here's how we could test for that:

test('clicking navigation link results in page contents changing', () => {
  const { getAllByRole, getByText, getByLabelText } = render(<App/>);

  // Click the Page 2 link inside the inner nav.
  // We could also just do `fireEvent.click(getByText('Page 2'))` but
  // this is an example, and the intent there is less clear anyway
  const navs = getAllByRole('navigation');
  // !!! New API !!!
  const toc = within(navs).getByLabelText('inner');
  fireEvent.click(toc.getByText('Page 2'));

  // now both the heading AND the inner nav have Page 2 in it.
  // but we only wanna verify that the heading has certain text
  // !!! New API !!!
  getByText(getAllByRole('heading'), 'Page 2'));
});

Instead of limiting a query to a single container, we can now query an array of containers. The end result looks very natural, hopefully!

Suggested implementation:

I'm happy to give this a go! I was thinking of adding this to the queryAllBy* methods:

// query-helpers.js
function createQuery(queryAllBy, container, ...args) => {
  if (Array.isArray(container)) {
    const elements = container
      // get an array of all the query results
      .map((el) => queryAllBy(el, ...args))
      // flatten the results into a single array
      .reduce((array, result) => array.concat(result), []);
    // dedupe
    return Array.from(new Set(elements));
  } else {
    return queryAllBy(container, ...args);
  }
};

// label-text.js
// I initially thought about adding this to `buildQueries` but then I saw that
// `*LabelText` does not follow the `buildQuery` pattern. So:
queryAllByLabelText = createQuery(queryAllByLabelText);

... And then add tests for it. Does that approach seem sound to you? I'd look deeper if I get the 👍 on whether the API change makes sense.

Describe alternatives you've considered:

We could implement the first query as getByRoleAndText(role, text), but it's a bit too specific, especially if we want to add more queries of that kind in the future. If dom-testing-library supports element arrays for the container, then these queries become natural to write without changing the API surface too much.

Teachability, Documentation, Adoption, Migration Strategy:

Migration and adoption should be easy since it's an addition to the query API. Documentation is a bit harder, but I imagine we can start stating that queries now can take an array of elements instead of just one element.

@alexkrolick
Copy link
Collaborator

alexkrolick commented Jun 23, 2019

It seems like you could get away with passing a function to the queries instead of this API change.

getByText((textcontent, node) =>
 getAllByRole('heading')
    .includes(node)
  && textcontent === 'Hi'
)

@kentcdodds
Copy link
Member

I don't think it makes sense to accept an array of containers. Thanks anyway!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants