Skip to content
This repository was archived by the owner on Jul 30, 2020. It is now read-only.

selector? option is not typed properly #71

Closed
ajsmth opened this issue Nov 3, 2019 · 4 comments · Fixed by #76
Closed

selector? option is not typed properly #71

ajsmth opened this issue Nov 3, 2019 · 4 comments · Fixed by #76
Labels

Comments

@ajsmth
Copy link
Contributor

ajsmth commented Nov 3, 2019

Looks like this option is implemented with the namespace 'filter' rather than selector, and the typescript definition flags this.

e.g in query-helpers.js:

Relevant code or config:

function queryAllByProp(
  prop,
  container,
  match,
  { filter = n => n, exact = true, collapseWhitespace, trim, normalizer } = {},
) { ... }

What you did:

I tried using the selector option for one of my custom queries

What happened:

Reproduction:

Example:

test.only('selector? bug', () => {
  const { baseElement, debug } = ntlRender(
    <Text accessibilityLabel="Hi" accessibilityStates={['disabled']}>
      Hi
    </Text>
  );

  // always finds because selector is not recognized as a filter
  const alwaysFinds = getByLabelText(baseElement, 'Hi', {
    selector: ({ props }) => props.acessibilityStates.includes('selected'),
  });

  debug(alwaysFinds);

  const willFind = getByLabelText(baseElement, 'Hi', {
    // @ts-ignore
    filter: ({ props }) => props.accessibilityStates.includes('disabled'),
  });

  debug(willFind);

  // throws as expected:
  const willNotFind = getByLabelText(baseElement, 'Hi', {
    // @ts-ignore
    filter: ({ props }) => props.accessibilityStates.includes('selected'),
  });

  debug(willNotFind);
});

Problem description:

I think this is just a naming issue between filter and selector

Suggested solution:

Either update the implementation to change the named parameter to be selector or update the typings / docs to reflect that the name should be filter

Can you help us fix this issue by submitting a pull request?

Yup

@bcarroll22
Copy link
Collaborator

Nice, if you can help troubleshoot this and get a PR up, I'll get it merged ASAP 👍 Thanks for pointing it out!

@ajsmth
Copy link
Contributor Author

ajsmth commented Nov 4, 2019

yup no problem -- do you prefer keeping the filter option or should I change it to selector? when i checked there was only ~3 functions with the filter named argument to update

@bcarroll22
Copy link
Collaborator

We should change it to selector to make sure it's consistent with the API of the rest of testing library (see here).

@bcarroll22
Copy link
Collaborator

🎉 This issue has been resolved in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging a pull request may close this issue.

2 participants