Fix: switch IS_REACT_ACT_ENVIRONMENT in userEvent

[pierrezimmermannbam]

Summary

I noticed on one of my project where I migrated fireEvent to userEvent that I got an act warning once when using userEvent.press. What I found is that it was due to the fact that the wait function does not use act, although it advances timers. This PR fixes that issue by using act in wait.

Test plan

I added a test case but it's not perfect. Before the fix there was a warning in the console but the test still worked. Because wait is async, the useEffect is still run even when not using act . The only solution I found to get the test to fail was to spy on console.error and make sure it's not called.

[codecov]

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (3be9f3b) 98.12% compared to head (59d0c14) 98.13%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1491      +/-   ##
==========================================
+ Coverage   98.12%   98.13%   +0.01%     
==========================================
  Files          96       97       +1     
  Lines        5704     5747      +43     
  Branches      885      890       +5     
==========================================
+ Hits         5597     5640      +43     
  Misses        107      107              

Files	Coverage Δ
src/user-event/press/press.ts	100.00% <100.00%> (ø)
src/user-event/setup/setup.ts	100.00% <100.00%> (ø)
src/waitFor.ts	81.18% <100.00%> (+3.99%)	⬆️
src/helpers/wrap-async.ts	67.44% <67.44%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mdjastrzebski]

@pierrezimmermannbam it seems to make sense to wrap wait in async act, similarly as we do e.g. with fireEvent, etc.

However, I've noticed that original User Event actually does not seem to use act at all, not for wait and not for dispatching events. I wonder how is that possible. Do you have any thoughts on that?

[mdjastrzebski]

I've found some clues about the original User Event, so since UE is not React specific it does not use act function but RTL passed eventWrapper(https://github.com/testing-library/react-testing-library/blob/c04b8f006c5a683d05c460c8ee1e2248d6f74350/src/pure.js#L59C7-L59C7) option to DTL.

The act function is later applied using wrapEvent helper to dispatchEvent. However, curiously UE does NOT wrap wait helper, and as far as I observer there is no code that would look like their wait is wrapped in act either. 🤔

[pierrezimmermannbam]

@mdjastrzebski that's interesting, I'll try to play with RTL's user event and see if I can reproduce the issue I encountered

[mdjastrzebski]

@pierrezimmermannbam is the issue you are trying to fix caused by this weird 130 ms delay introduced by React Native? If so then perhaps it would be good idea to just wrap that one wait call with async act.

I think the idea behind not wrapping wait with act in the OG User Event ight have to do with UE not expecting any async code to be hit these pauses, which by default are 0ms, but in case of press its actually 130 ms.

[pierrezimmermannbam]

I've done some testing with RTL using about the same test:

it.only('is wrapped by act', async () => {
  const consoleErrorSpy = jest.spyOn(console, 'error');
  jest.useRealTimers();
  const TestComponent = () => {
    const [isVisible, setIsVisible] = React.useState(false);

    React.useEffect(() => {
      setTimeout(() => {
        setIsVisible(true);
      }, 100);
    }, []);

    return (
      <>
        <input placeholder="toto" />
        {isVisible && <span>Visible</span>}
      </>
    );
  };

  render(<TestComponent />);

  await userEvent.type(screen.getByPlaceholderText('toto'), 'al', {
    delay: 200,
  });
});

There is no act warning and what I've found is that the asyncWrapper is used so my guess is that every async operation is wrapped by act so the wait will be wrapped as well.

In our case it's not linked to the delay for the press out, any async code could be run while the user performs an action, as demonstrated in the test case I wrote in this PR, so I think it's a valid use case for using act

[mdjastrzebski]

@pierrezimmermannbam asyncWrapper (drain microtask queue while preserving act env) and eventWrapper (wrap with act) seem to be different tools. As far as I understand they wrap each interaction in asyncWrapper (here).

More interesting, they seem to add a dummy wait call after each interaction: here

[pierrezimmermannbam]

@mdjastrzebski you're right, maybe we could try a similar approach with userEvent? This way we could make sure that every interaction is wrapped by act and prevent similar bugs from happening. Interesting also that they add a call to wait at the end, I guess we could mimic that behavior as well, wdyt?

[mdjastrzebski]

@pierrezimmermannbam afaik each interaction in UE is not wrapped by act (eventWrapper) but by asyncWrapper which drains microtask queue without invoking act.

Not sure about the purpose of that wait at the end.

[pierrezimmermannbam]

@mdjastrzebski, looking again at this RFC on IS_REACT_ACT_ENVIRONMENT , it looks like we should only use act for interactions and not when waiting (e.g. when using waitFor or wait). Also I saw an act warning being thrown in a codebase when using userEvent.type and removing the calls to wait didn't fix the issue so the previous fix was not really relevant.

I updated the PR to mimic the behavior from RTL. We wrap every API from userEvent with the same async wrapper we use for waitFor which would be similar to their asyncWrapper and we keep act in dispatch event, which would play the role of RTL's eventWrapper.

I was not too sure how to test this, I added a test case where an act warning would have been thrown if act environment wasn't set to false. I doesn't test every API but I don't really think it's worthwhile to write a test for every API. If this doesn't work as expected then we could add more tests if we feel they're valuable.

I also kept the wait in wrapAndBindImpl. It makes sense to wait between each user interaction imo and the closer our API is from RTL's the better

[mdjastrzebski]

Seems like we could move it to utils next to asyncWrapper

[pierrezimmermannbam]

Actually I moved the asyncWrapper cause it's used also by waitFor so I don't think it should be in the userEvent folder. I don't see wrapAndBindImpl being used elsewhere as it's directly tied to user event so I think it's in the right place

[mdjastrzebski]

So, we only wrap in act from React 16.9 to 17.x. Pls add code comment clarifing this

[mdjastrzebski]

If it's RNTL-wide helper, move it to general utils from user-event/utils

[mdjastrzebski]

RTL has addtionally jest advance timers call: https://github.com/testing-library/react-testing-library/blob/c04b8f006c5a683d05c460c8ee1e2248d6f74350/src/pure.js#L49

Do you think we should add this as well?

[pierrezimmermannbam]

Not sure at all about that, I have no clue what it does. My thought process here was that we already had pretty much our own version of asyncWrapper that we use for waitFor and it seems to be working pretty well so I figured we might as well stick to that version. Maybe it could be worth investigating what difference it could make to get closer to RTL's implementation but I'd rather do that in another PR

[mdjastrzebski]

@pierrezimmermannbam thanks for investigating that. I like that new implementation got in line with RTL,s one. I've left some minor comments to be addressed before merging.

NB: From reading the code it seems like we are only focusing on supporting React 18, with React 17 logging some errors in the console, so it would be worth considering dropping official React 17 support in RNTL v13 #1505, since we aren't actually supporting it.

[pierrezimmermannbam]

@mdjastrzebski thanks for the review! I addressed your comments. I agree with you on React 17 support, there are some tests breaking and some errors logged as well on the main branch. And this PR also makes things a bit worse because it introduces overlapping act calls. We could fix that however by removing act in dispatchEvent for versions < 18, but at this point I'm not even sure it'd be worth it, wdyt?

[pierrezimmermannbam]

@mdjastrzebski could you please review when you have the time? It would be nice to have this pr in a release soon because this is currently a big blocker for user event

[mdjastrzebski]

@pierrezimmermannbam Thanks for pinging me, I've been pretty busy lately, but I'll try to review this in the following days.

[mdjastrzebski]

(rebased on main)

[mdjastrzebski]

Looks good to me!

[mdjastrzebski]

This PR has been released v12.3.1 🚀