@testing-library/react-hooks specific rules

[mpeyper]

I'd like to propose a new rule for a testing-library that I do not believe is currently covered in this plugin. I'm not sure if it would be part of testing-library/react rules or if a new set of rules (testing-library/react-hooks) should be introduced. Reading this, it's very possible that people will be using both @testing-library/react and @testing-library/react-hooks in the same project and want to have it in the same eslint config, so not sure how it would be best structured.

There is a common mistake people make when using @testing-library/react-hooks that should be pretty simple to lint for. The issue looks like this:

const { result } = renderHook(() => useCounter());

const { value, increment } = result.current;

expect(value).toBe(0);

act(() => {
  increment();
})

expect(value).toBe(1);

Assuming useCounter works as intended, this test would fail because they have destructured result.current.value, effectively locking its value to whatever it was when initially rendered.

(To be clear, people generally aren't testing the value before the act they are just asking why it's wrong at the final expect, but I wanted to be clear about where it goes wrong)

The test should be written as:

const { result } = renderHook(() => useCounter());

expect(result.current.value).toBe(0);

act(() => {
  result.current.increment();
})

expect(result.current.value).toBe(1);

This allows result.current.value to be updated when the internal component renders.

This logic is true for increment as well as this test would also fail because increment is using a stale closure for the update:

const { result } = renderHook(() => useCounter());

const { increment } = result.current;

expect(result.current.value).toBe(0);

act(() => {
  increment();
})

expect(result.current.value).toBe(1);

act(() => {
  increment();
})

expect(result.current.value).toBe(2);

There would be very few cases where destructuring any value from result.current is needed. The only one that I can think of is if you wanted to capture the initial value to test that the new value was different (e.g. a randomly generated value), but in my experience, this is not very common. However, if the test does not perform any updates, the test could still pass, therefore making the destructuring technically valid, but in those cases, inlining result.current would also work.

Let me know what you think, if it's possible and if I can help in any way towards implementing it. I've never worked on an eslint plugin before, but I'm willing to learn.

[Belco90]

Hey @mpeyper! Thanks for creating this issue, a really interesting idea (I've made this mistake many times).

I think it could be included in this plugin, but there are several things to handle:

• I wouldn't worry about the Shareable Configs note where we warn about not mixing more than one. Technically it's possible to do so, we only wanted to avoid people enabling both dom and react one since they are exclusive between them. We can update that message to indicate react-hooks is an additional one that can be enabled together with react one.
• By the previous point, we can add a new react-hooks Shareable Config and enable this new rule. We need to make sure we mention in the rule's doc that it's exclusive for that particular Testing Library framework (not the first time this happens, here you have an example of a rule exclusive for some framework)
• We might need to rework how the plugin detects if a file has Testing Library utils to be reported. IIRC you won't use both @testing-library/react and @testing-library/react-hooks in the same file right? (this could be a potential rule? 😅 ) Assuming this, we wouldn't need to change anything here for a first approach but would be interesting to improve it in the future to differentiate between Testing Library packages and react-hooks one
• The rule itself seems simple. We would need to add new detection helpers for renderHook and result tho (making sure the existing helpers for detecting render related stuff don't conflict with them, otherwise renderHook would be reported by Testing Library render related rules)

Sorry, thinking a little bit out loud here! To sum it up: I think it makes sense to include rules related to @testing-library/react-hooks in this plugin (actually pretty excited about it), but we need to improve the base detection mechanisms + new Shareable Config to do so.

What do you think?

[mpeyper]

Sounds good to me. I'm not sure where to start, but let me know how I can help.

IIRC you won't use both @testing-library/react and @testing-library/react-hooks in the same file right?

While it would be uncommon, there's technically no reason why you can't. I have seen examples of context providers being rendered with RTL and hooks consuming the context being rendered with RHTL in the same file, or making their own render helpers around render and renderHook what use common providers for both that all live together.

[Belco90]

Sounds good to me. I'm not sure where to start, but let me know how I can help.

I think you can start by creating the necessary rule files first (it's a shame we don't have the scaffolding in place for it yet, but CONTRIBUTING should guide you through it) and extending SUPPORTED_TESTING_FRAMEWORKS to include react-hooks one (you'll have to update all existing rule configs to set it as false). When you have that in place, you can implement the rule itself, and then we will make sure the current detection mechanisms are updated as mentioned before

You have tons of different utils in the codebase already:

• node-utils: this module contains several functions that will help you visiting AST nodes, navigating through them, checking their type etc
• detection helpers: this bunch of helpers is injected as the 3rd argument in create method of your rule when creating it with createTestingLibraryRule. These helpers will help you to check things related to Testing Library: is this Identifier a render util? is that node a findBy* query? etc. I don't think there is any helper you can reuse since none of them aim to detect render-hooks stuff. However, this is where we will need to add the new detection helpers for renderHook and result, but it's ok for now if you just try to detect them within your rule itself, and then I'll help you to move it here.

Additionally, I highly recommend you to have AST Explorer open so you can figure out the proper AST structure etc.

Are you happy with this approach?

IIRC you won't use both @testing-library/react and @testing-library/react-hooks in the same file right?

While it would be uncommon, there's technically no reason why you can't. I have seen examples of context providers being rendered with RTL and hooks consuming the context being rendered with RHTL in the same file or making their own render helpers around render and renderHook what use common providers for both that all live together.

Then we definitely need to separate current Testing Library imports detection from React Hooks Testing Library imports. We will figure that out when you have made some progress in the rule itself and we start moving things to detection helpers.

[mpeyper]

Thanks for the pointers. I'll try to get started on this soon, but if anyone else reading this wants to take it on, please feel free to do so. I'm also available to give input, opinions and reviews for it if that helps.

[Belco90]

Cool! I'm busy with other issues at the moment, but I can take care of this one when I handle all of them.

Leaving here more pointers for the rule implementation:

• I think it could be called no-destructure-result-current
• The basic idea for detecting the code violation would be:
  1. Look for destructured values: VariableDeclarator with id of type ObjectPattern
  2. From nodes obtained before, check if their init comes from a reportable renderHook. It's not enough just checking the init is named "result.current" since 1) that doesn't mean is related to renderHook and 2) could have been renamed
• We need to make sure we cover edge cases like const { result: { current: increment } } = renderHook(() => useCounter());

[mpeyper]

I know I've used destructuring in all my examples, but the same would be true for a regular old variable declaration:

const value = result.current.value;

I assume that would still be a VariableDeclarator but with a different id type?

[Belco90]

Of course, 🤦 that's a really good point!

Indeed, that's a VariableDeclarator with an id of type Identifier.

You can compare both here: https://astexplorer.net/#/gist/4fc3a82a692a2a82cab52e7c8371b2bb/66e9e82eab4a9814cadd22631364847b0e2ad1bc

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[mpeyper]

This is still on my todo list, but I haven't had much time to look at it recently and not sure when I will have time.