add no-unnecessary-act rule

[kentcdodds]

We should warn against the use of act unnecessarily:

import {screen, fireEvent, render, act} from '@testing-library/react'
import userEvent from '@testing-library/user-event`
import ReactTestUtils from 'react-dom/test-utils'

// ReactTestUtils.act should be treated exactly the same as `act` from `@testing-library/react`

// these are ALL wrong:
act(() => fireEvent.click(el))
act(() => screen.getByText('blah'))
act(() => userEvent.click(el))
await act(async () => userEvent.type('hi', el))
act(() => render(<div />))
await act(async () => render(<div />))
await act(async () => {})
act(() => {})

// this one's fine:
act(() => {
  stuffThatDoesNotUseRTL()
})

// actually, so is this one
act(() => {
  fireEvent.click(el)
  stuffThatDoesNotUseRTL()
})

So basically the rule is: "You should only wrap non-testing-library function calls in act"

Reading material: https://kcd.im/react-act

[Belco90]

Hi @kentcdodds, thanks for the proposal! This would be tricky to implement on current v3 where we have some issues to detect testing-library related things. However, we got a new detection mechanism on v4 (still in progress) which will be perfect for implementing this one. I think we will include it in a possible v4.1 or maybe within v4 depending on how we are progressing with last bits of v4.

If someone wants to take care of this one meanwhile, please do it by branching off v4.

Thanks.

[Belco90]

Hi again @kentcdodds. In the twitter thread were you explained this behavior you mentioned that "warning would go away just as well if you added await act(async () => {}) on the next line" so I guess that would be another invalid case, wouldn't be?

[kentcdodds]

Yes! An empty act(() => {}) or await act(async () => {}) should also be invalid 👍

[alessbell]

👋 I can pick this one up and branch off of v4 to get started if no one else has yet @Belco90

[Belco90]

@alessbell That would be awesome! I don't think anyone has started it yet. I'm busy migrating current rules to v4 (you can see progress on #198 ) so didn't have time to even think about this one.

We haven't updated the contributing guidelines yet for v4, but some guidance around it:

• there is a new detection mechanism on v4 which injects a 3rd argument to every rule create method. You can take advantage of that to detect if something is related to testing library or not
• even if we have those new helpers for detecting things, you might need something which doesn't exist yet. If so, feel free to add new helper to detection mechanism
• for this rule you can use isNodeComingFromTestingLibrary helper which will automatically tell you if a particular node comes from something related to testing library or not!
• I don't think we need to check if act is coming from testing library or ReactTestUtils and just assume every act call we find has to be reported if wrapping something coming from testing library

Let me know if you have any question around v4 or you don't know how to proceed with something! Sorry we don't have the guidelines updated yet, but I think the detection method and helpers are well documented.

[Belco90]

Hey @alessbell just FYI: I've merged #263 into v4 branch, and I created #265 pointing to v4 too. The former shouldn't affect you directly, but the latter includes some refactors within isNodeComingFromTestingLibrary. It should work in the same way (even covering more edge cases), but just to let you know about it in case you were modifying that util too!

[zaicevas]

Hey @alessbell how are things going with this task? Are there any blockers?

[alessbell]

Thanks for bumping this thread @tozaicevas -- I have an in-progress branch I've just rebased off of the latest v4 and should be able to clean it up this evening 👍 Will open a PR shortly.

[Belco90]

@alessbell thanks for the update! I created a PR yesterday to move the CI to GitHub Actions. I'll merge it later today, and try to get it updated properly on v4 so we can have proper CI in place for your upcoming PR and current ones open for v4.

Also, I've updated tons of things on v4 since my last update here. Feel free to ping me in you need help or just want to know more about it.

Looking forward to your PR, thanks for your work :)

[Belco90]

@alessbell forgot to mention it, but the new CI is already moved to GitHub Actions in both main and v4 branches ✨

[Belco90]

Hey @kentcdodds! One question about your original description of the behavior:

act(() => {
  fireEvent.click(el)
  stuffThatDoesNotUseRTL()
})

You said in the description this should be a valid usage of act, are we sure about it? I thought this should be an invalid example since fireEvent is a Testing Library util, so previous example should be reported until all Testing Library utils are extracted outside the act like:

fireEvent.click(el)
act(() => {
  stuffThatDoesNotUseRTL()
})

[kentcdodds]

It would be valid I think because it will make sure that effects aren't flushed between the fireEvent and the stuffThatDoesNotUseRTL. I can't really think of a situation where you'd want that, but a situation like that probably exists.

[Belco90]

Hey hey! Since v4 was officially released and the first round of improvements after that was handled too, I have time for implementing other rules like this one. @alessbell will hand her current progress over to me so I can finish it.

[github-actions]

🎉 This issue has been resolved in version 4.3.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀