refactor: flush microtask queue implementation

[mdjastrzebski]

Summary

Edit
This PR renames the old flushMicroTasks functions as flushMicroTasksLegacy, and adds a proper one under the name of flushMicroTasks. Respective implementations are invoked in the sample places as before, so there should be no behavior change for the users. We will delay removal of flushMicroTasksLegacy until the next major release, to avoid introducing breaking changes mid-cycle.

Original
PR to fix flushMicrotaskQueue implemantion based on @jsnajdr comment:

This is a weird thenable because the then function doesn't return anything and is therefore not chainable. Also, the setImmediate timer starts running at the time when the promise chain is constructed, not when it runs.

flushMicroTasks().then( flushMicroTasks ).then( flushMicroTasks )
should run there setImmediates after each other, but instead it will crash, and even after fixing the crash, it will run all three timers in parallel.

The way how await works doesn't expose these bugs, but outside await, the helper won't work.

My intuition is that the really correct version will look differently 🙂

This is the implementation in question:

export function flushMicroTasks(): Thenable<void> {
  return {
    // using "thenable" instead of a Promise, because otherwise it breaks when
    // using "modern" fake timers
    then(resolve) {
      setImmediate(resolve);
    },
  };
}

Test plan

All current tests should pass.

Reviewers

@jsnajdr pls review and share you comments.
@mikeduminy, @dcalhoun adding you, as you seem knowledgeable in this area. Could you pls share your comments and/or concerns.

[jsnajdr]

Yes, this is the most straightforward and correct way to wait for setImmediate. But the original comment said that this implementation breaks with modern fake timers? That turns out to be not true, doesn't it?

By the way, there's one more setImmediate await that could be replaced with flushMicroTasks, in the waitForInternal function.

[thymikee]

Can you test it with older versions of Jest and see where they fixed the implementation?

[mdjastrzebski]

@thymikee could you take a look, the current implementation seems to come from you in PR #397 resolving issues #391. That does not seems to be needed any more as we have various tests with both modern and legacy fake timers, and all pass with more "standard" way of flushing microtask queue.

[jsnajdr]

I tried to debug why the new Promise(r=>setImmediate(r)) implementation was failing with modern timers, and I tracked it down to this React Native PR: facebook/react-native#34659

Before that PR, React Native's jest/setup.js file was installing a Promise polyfill from the promise NPM package. And that package internally calls process.nextTick, which is being hijacked by modern fake timers. Somebody had to advance the fake timers just to resolve promises and call their then handlers. And nobody was making that call.

With recent versions of React Native, and the current react-native-testing-library depends on a recent-enough version, that polyfill is no longer used, and all promises are native. Modern fake timers no longer break them.

[thymikee]

@mdjastrzebski we might need a note somewhere that this is only compatible with newer version of RN

[jsnajdr]

That Promise polyfill removal was shipped in React Native 0.71, in January 2023, just a few weeks ago. And this package has a peer dependency on React Native >=0.59, keeping back compat back to November 2021. It needs to keep the weird thenable until the peer dep gets updated one day.

Or, alternatively, it could advance the timers, like:

new Promise(resolve => {
  setImmediate(() => {
    resolve();
    if (jestFakeTimersEnabled()) {
      jest.advanceTimersByTime(0);
    }
  });
});

The @testing-library/react library does something similar, and we discussed the need for advanceTimersByTime in #1366 (comment).

Here the situation is a bit different, the timers need to be advanced after resolve has been called. The microtask queue needs to be already populated when we're flushing it.

[dcalhoun]

@dcalhoun adding you, as you seem knowledgeable in this area. Could you pls share your comments and/or concerns.

Thanks for the ping. I do not have any specific concerns at this time.

@jsnajdr's original concern makes sense. Also, he has done a fair amount of in-depth research into our project's tests. I believe he has a great understanding of the domain.

Additionally, the tests (added in #568) covering one of my original discoveries related to Promise and fake timers appear to still be present and passing in this PR. 🎉

[mdjastrzebski]

I've done some tests with our current setup (which includes custom jest preset saving the Promise):

• RN: 0.71.4, R: 18.2.0, Jest: 29.4 - all tests pass

• RN: 0.71.4, R: 18.2.0, Jest: 28.0 - all tests pass

• RN: 0.71.4, R: 18.2.0, Jest: 27.0 - all tests pass (after setting testEnvironment: "node" in Jest config file)

• RN: 0.70.0, R: 18:1.0, Jest: 29.4 - all tests pass

• RN: 0.70.0, R: 18:1.0, Jest: 28.0 - all tests pass

• RN: 0.70.0, R: 18:1.0, Jest: 27.0 - all tests pass

• RN: 0.69.10, R: 18.0.0, Jest: 29.4 - all tests pass

• RN: 0.69.10, R: 18.0.0, Jest: 28.0 - all tests pass

• RN: 0.69.10, R: 18.0.0, Jest: 27.0 - all tests pass

When I swap the preset to preset: "react-native" then waitFor, timers and auto-cleanup tests starts to fail:

• RN: 0.71.4, R: 18.2.0, Jest: 29.0 - all tests
• RN: 0.70.0, R: 18.1.0, Jest: 29.0 - waitFor, timers and auto-cleanup tests fail
• RN: 0.70.0, R: 18.1.0, Jest: 27.0 - waitFor, timers and auto-cleanup tests fail

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 🎉

Comparison is base (ce75f66) 96.88% compared to head (ffaa8df) 96.89%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1374      +/-   ##
==========================================
+ Coverage   96.88%   96.89%   +0.01%     
==========================================
  Files          65       65              
  Lines        3693     3709      +16     
  Branches      538      539       +1     
==========================================
+ Hits         3578     3594      +16     
  Misses        115      115              

Impacted Files	Coverage Δ
src/flush-micro-tasks.ts	100.00% <100.00%> (ø)
src/index.ts	100.00% <100.00%> (ø)
src/waitFor.ts	77.19% <100.00%> (-0.10%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mdjastrzebski]

@jsnajdr @thymikee I've updated the PR with a different approach when we delay the release of changes to flushMicroTask to the next major release. This PR only contains a slight code organization changes, where original flushMicroTasks becomes flushMicroTasksLegacy with comments about deprecation and removal in the next major release. At the same time I've added proper flushMicroTask implementation and applied it to previous instances of flushing microtasks with new Promise() + setImmediate. This way the user behavior should not change.

[jsnajdr]

Looks like a good preparation for future deprecation 👍

[mdjastrzebski]

This PR has been released in v12.1.3 🚢