Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[React 19] use is significantly slower in some scenarios than throwing a Promise #29855

Open
phryneas opened this issue Jun 11, 2024 · 10 comments
Labels

Comments

@phryneas
Copy link
Contributor

Summary

This is something I noticed when I applied the Apollo Client test suite to React 19 - we had one test that was timing out.

I first thought that the test was broken, but it turns out it was just really slow.

Cranking the test that finished in React 18 in under 200 milliseconds to a timeout of 10 seconds for React 19 made it pass (6 seconds would still consistently fail, 7 seconds started to pass on localhost).

Further I noticed that if we use our __use polyfill that throws a Promise instead of use, the test finishes in under 500 milliseconds - still slower than React 18, but significantly faster than React 19 with native use.

This is the test in question:

https://github.com/apollographql/apollo-client/blob/8e3edd4f5e5191453ba3ca1a1cc4fc4a02b936e8/src/react/hooks/__tests__/useSuspenseQuery.test.tsx#L9519-L9615

I'd be happy to work with you on further identifying the root cause for this, but I'll be moving houses in a few days, and I just don't have the time to create an isolated reproduction for this within the next 1-2 weeks, I'm really sorry.

Still, I assume this might be important for you, so I'll at least open a ticket to let you know :)

@phryneas
Copy link
Contributor Author

phryneas commented Jun 12, 2024

Added context: this seems to be triggered by user.type(input, "ab")); - changing it to user.type(input, "a")); makes everything fast again.

So my working theory is that the order of operation

  • user types "a"
  • useDeferredValue starts a transition and a query for "a" creates a promise that is passed into use
  • user types "b"
  • useDeferredValue starts a transition and a query for "ab" creates a new promise that is passed into use

seems to cause this - maybe because use still latches onto the first promise and ignores the second, semantically different, promise.

@jerelmiller
Copy link

I'll add to this some additional findings. @phryneas's theory is close, but it seems this only occurs when the original value is updated before the deferred value. Everything stays fast if the deferred value only lags behind one update from the original. In other words, if I log the two values, the fast execution has this timeline:

{ value: 'a', deferredValue: '' }
{ value: 'a', deferredValue: 'a' }
{ value: 'ab', deferredValue: 'a' }
{ value: 'ab', deferredValue: 'ab' }

which I can simulate with await user.type('a'); await user.type('b') (using the userEvent library).

But it gets slow with this timeline (i.e. the await user.type('ab') case):

{ value: 'a', deferredValue: '' }
{ value: 'ab', deferredValue: '' }
{ value: 'ab', deferredValue: 'ab' }

The promise attached to the empty string is resolved by the time we simulate the ab keyboard input and we only create a single promise when deferredValue gets updated to ab, so I can confirm we aren't accidentally creating a bunch of new promises that use can't keep up with.

I'm still working to see if I can create an isolated reproduction without Apollo or its test suite. Will report back if I have success.

@jerelmiller
Copy link

jerelmiller commented Jun 13, 2024

Update: It seems the issue lies in the way act works with the user event library, specifically typing multiple characters. If I replace this line:

await act(() => user.type('ab'))

with

await user.type('ab')`

I see act warnings, but the test goes back to being fast. Seems there is some combination of things here that cause something to spiral out of control. Here are all the things that work to speed up the test again and exhibit the expected behavior:

  • Removing act around the user.type('ab')
  • Typing a letter at a time (i.e. await act(() => user.type('a')); await act(() => user.type('b')))
  • Switching React.use for our custom __use polyfill

If the swap of React.use didn't do anything, I'd be inclined to say the the problem was with the user event library, but perhaps the combination of act and use is doing something odd here 🤔.

I'll continue working on a reproduction in a test environment. I was unable to reproduce using a running app in the browser which is a good thing.

@jerelmiller
Copy link

Alright I was able to get a reproduction together that demonstrates the issue in a test. Check out my reproduction here: https://github.com/jerelmiller/react-use-reproduction/blob/master/src/App.test.tsx

Run npm test to run the test and you'll see it timeout. I've annotated that test file with comments that describe how to get the test to pass without timing out. This structure is consistent with what we are seeing in Apollo Client's codebase. The test will pass eventually if you increase the timeout high enough, but there is no reason it shouldn't pass within the default 5s window.

@RacketyWater7
Copy link

RacketyWater7 commented Jun 23, 2024

Thanks for the detailed investigation, @phryneas and @jerelmiller! The repro you put together (https://github.com/react-native-community/reproducer-react-native) is really helpful in understanding the root cause.

It seems the slowness is related to how use interacts with promises generated from multiple user input events within a single act call, specifically when the original value updates before the deferred value. This is interesting behavior, and I'd be glad to help troubleshoot further.

Here are some additional thoughts based on your findings:

Consider alternative testing approaches: While act is useful for simulating user interactions, could the test be rewritten to avoid the problematic combination of act and multiple user input events within it? Perhaps separate act calls for each user input could be explored.
Investigate the root cause within use: If the issue lies within the use hook itself, diving deeper into the implementation might reveal areas for optimization, especially regarding handling multiple promises triggered by user interactions.

I'm happy to assist in any way possible to move this issue forward. Perhaps we can discuss potential solutions or debugging strategies in a follow-up comment or collaboratively on a pull request if a code change is identified.

@phryneas
Copy link
Contributor Author

phryneas commented Jun 23, 2024

@RacketyWater7 This is a bug report in the React repo.
We know how to work around this and do not need assistance with that.

Trying to work around the problem we are reporting here would be counterproductive to the the intent of this issue - identifying the bug in React, reproducting it and getting it fixed.
We literally spent two days to get a reliable isolated reproduction of this bug.

@phryneas
Copy link
Contributor Author

I could narrow it down a bit further by replacing user.type with manual event dispatches.

This pattern is problematic in combination with use and useDeferredValue:

  await act(async () => {
    fireEvent.change(input, { target: { value: "a" } });
    await new Promise((resolve) => setTimeout(resolve, 1));
    fireEvent.change(input, { target: { value: "ab" } });
  });

If you skip the act, it works as expected, if you skip the setTimeout between the events it also works as expected.

@eps1lon
Copy link
Contributor

eps1lon commented Jun 25, 2024

@phryneas Can you pack this into a minimal, cloneable repro? Ideally with clear instructions how to repro and what the expected vs actual behavior is for each case.

@phryneas
Copy link
Contributor Author

phryneas commented Jun 25, 2024

@eps1lon we updated the repro that @jerelmiller linked above: https://github.com/jerelmiller/react-use-reproduction/blob/master/src/App.test.tsx

For running it, just clone it and run yarn install followed by yarn test -- the file with the reproduction is only 100 lines and half of them are comments :)

@jerelmiller
Copy link

@eps1lon I also updated the README of that repo to include the details and reproduction steps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants