[prefer-find-by] When query methods come from destructuring, findBy* might not be defined

[gndelia]

Example code before running the linter

const { getByText } = render(<Foo />)
await waitFor(() => getByText('foo'))

After automatically fixing this, the code looks like this

const { getByText } = render(<Foo />)
await findByText('foo')

There are a couple of issues:

1. As the title mentions, findByText is not defined. If the code was using screen or any similar wrapper, instead of the bound function directly, this is not a problem.
2. If getByText is not used elsewhere, it becomes unused, causing another error in the linter that wasn't there before - we've introduced this error
3. Something similar might happen with waitFor - it might now be unused as well

We should definitely fix 1. as it otherwise it's a broken code. I guess I could track where getByText was defined, and add the findBy* function there.

Thoughts on 2. and 3 ?

[nickserv]

I guess we could look up where it was destructured from and change the properties that are destructured rather than just the named of the called functions?

[Belco90]

From ESLint rules fixes best practices, 1 is a bug here, but 2 and 3 aren't.

Avoid any fixes that could change the runtime behavior of code and cause it to stop working.

👆 Because of this, 1 is a bug as it can cause the code to stop working as findBy is not imported.

Since all rules are run again after the initial round of fixes is applied, it's not necessary for a rule to check whether the code style of a fix will cause errors to be reported by another rule.

👆 Because of this, 2 and 3 are not an issue. A rule should not take care of possible errors reported by other rules. And they are not causing the code to stop working.

[gndelia]

I thought so. Ok so this ticket should be used to track the fixing of 1. Thanks!!

[Belco90]

How are you planning to fix it? I have couple of ideas, but it can get complex for some edge cases like:

const setUp = () => render(<Foo />);

const { getByText } = setUp();

await waitFor(() => getByText('submit'));

We can't just destructure from render in this case. Maybe just checking where the original query being replaced (e.g. getByText) was destructured is enough to include the new query (e.g. findByText)? What if already imported? I guess we need to check this within the function scope where we are fixing the code.

[gndelia]

the best scenario seems to look up the place where the original query was restructured from, and replace it there if it wasn't already.

We will have to set up a good amount of tests to verify all scenarios work as expected

[Belco90]

Cool, we are in the same page then. Indeed, we need a nice bunch of tests here. Apart from that case mentioned before, I can think about these additional ones in case it helps:

// keeping render returned queries in `view` variable
const view = render(<Foo />);

await waitFor(() => view.getByText('submit'));

// same as previous one but from setup method wrapping render
const setUp = () => render(<Foo />);

const view = setUp();

await waitFor(() => view.getByText('submit'));

// findByText already destructured so it shouldn't be added here
const { getByText, findByText } = render(<Foo />);

await waitFor(() => getByText('submit'));

// same as previous one but from setup method wrapping render
const setUp = () => render(<Foo />);

const { getByText, findByText } = setUp();

await waitFor(() => getByText('submit'));

[gndelia]

just to clarify, the rule does not cover expects inside. The general form should be more like

await waitFor(() => getByText('submit'))

[Belco90]

My bad, I'll update the examples.

[gndelia]

// keeping render returned queries in `view` variable
const view = render(<Foo />);

await waitFor(() => view.getByText('submit'));

// same as previous one but from setup method wrapping render
const setUp = () => render(<Foo />);

const view = setUp();

await waitFor(() => view.getByText('submit'));

these two should work, as the replaced code will use findByText from view: view.findByText('submit'). Can't remember if there are tests that cover this (I think there are, but using utils as the name instead of view)

// findByText already destructured so it shouldn't be added here
const { getByText, findByText } = render(<Foo />);

await waitFor(() => getByText('submit'));

// same as previous one but from setup method wrapping render
const setUp = () => render(<Foo />);

const { getByText, findByText } = setUp();

await waitFor(() => getByText('submit'));

yes, we should cover these

[gndelia]

I'm taking this one