fix(wait-for): Don't queue microtasks after condition is met

[eps1lon]

What:

Fixes integration with of @testing-library/react@alpha in React Spectrum when using React 18.

Why:

Otherwise context restoration in unstable_advanceTimersWrapper becomes unpredictable leading to fixes like d375ff3 (#2526) that seemingly have nothing to do with the test/implemenation but magically fix tests.

The implementation change fixed

it('should handle when a non drop target element is added', async () => {
      let setShowInput2;
      let Test = () => {
        let [showInput2, _setShowInput2] = React.useState(false);
        setShowInput2 = _setShowInput2;
        return (<>
          <Draggable />
          <input />
          <Droppable />
          {showInput2 &&
            <input />
          }
        </>);
      };

      let tree = render(<Test />);

      let draggable = tree.getByText('Drag me');

      expect(tree.getAllByRole('textbox')).toHaveLength(1);

      fireEvent.focus(draggable);
      fireEvent.click(draggable);
      act(() => jest.runAllTimers());

      expect(() => tree.getAllByRole('textbox')).toThrow();

      act(() => setShowInput2(true));
      // MutationObserver is async
      await waitFor(() => expect(() => tree.getAllByRole('textbox')).toThrow());

      fireEvent.click(draggable);
      expect(tree.getAllByRole('textbox')).toHaveLength(2);
    });

-- /~https://github.com/adobe/react-spectrum/blob/8dc33dcdab3bb41518daf023c18939e844e843eb/packages/%40react-aria/dnd/test/dnd.test.js#L2072-L2105

How:

Exit the loop once the callback no longer throws instead of unconditionally queuing another microtask.

This may be an issue with how @testing-library/react uses unstable_advanceTimersWrapper.
Alternatively, we could flush microtasks before we check the callback. But this was intentionall though we don't have any test/repro for that at the moment /cc @kentcdodds

Checklist:

• Documentation added to the
  docs site
• Tests
• [ ] TypeScript definitions updated
• Ready to be merged
  

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2b8da94:

Sandbox	Source
react-testing-library-examples	Configuration

[codecov]

Codecov Report

Merging #1073 (2b8da94) into main (2866544) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1073   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines          952       954    +2     
  Branches       311       312    +1     
=========================================
+ Hits           952       954    +2     

Flag	Coverage Δ
node-12	100.00% <100.00%> (ø)
node-14	100.00% <100.00%> (ø)
node-16.9.1	100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/wait-for.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2866544...2b8da94. Read the comment docs.

[eps1lon]

Was no-act without the change. We had the following schedule-resolution order:

Without this change:

1. start asyncWrapper
2. start unstable_advanceTimersWrapper
3. leave asyncWrapper
4. leave unstable_advanceTimersWrapper

With this change:

1. start asyncWrapper
2. start unstable_advanceTimersWrapper
3. leave unstable_advanceTimersWrapper
4. leave asyncWrapper

In other words, before asyncWrapper and unstable_advanceTimersWrapper were interleaved instead of nested.

[kentcdodds]

I didn't realize unstable_advanceTimersWrapper was a thing 😅 The test looks a bit complicated, but I understand why so that's fine. This looks good to me 👍

[eps1lon]

I didn't realize unstable_advanceTimersWrapper was a thing sweat_smile

Added in #1022 because we can't indiscriminantly wrap waitFor in act anymore in React 18.

I hope the linked issues/threads etc make it possible to follow why we need all of this. Though it's quite involved. Please hit me over the head after React 18 is stable that I should write up why we needed all the changes. Right now I'm too lazy considering all of this may change.

The test looks a bit complicated, but I understand why so that's fine. This looks good to me +1

Yeah the test is way too involved for my liking as well. I'm just glad we have some automatic verification though. I wouldn't be surprised if it turns out that the usage of unstable_advanceTimersWrapper is just wrong in the test. But at least this way we have accountability.

[eps1lon]

RE: #667 (comment)

Bookshelf app tests path with this PR (#1073).
But they fail if I revert #667 so let's stick with the approach in this PR.

Ultimately we should add a test that illustrates what's happening in the bookshelf app. Hopefully we can refactor our implementation to fix all tests but keep the code simpler.

[kentcdodds]

Sounds good to me 👍

[github-actions]

🎉 This PR is included in version 8.11.1 🎉

The release is available on:

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

Your semantic-release bot 📦🚀