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

Timeline: Nested update warning logic flaw #22731

Open
bvaughn opened this issue Nov 9, 2021 · 4 comments
Open

Timeline: Nested update warning logic flaw #22731

bvaughn opened this issue Nov 9, 2021 · 4 comments

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Nov 9, 2021

Timeline shows the following nested update warning when a synchronous update causes an event handler to run long:

A big nested update was scheduled during layout. Nested updates require React to re-render synchronously before the browser can paint. Consider delaying this update by moving it to a passive effect (useEffect).

This warning was intended to encourage developers to move heavy updates from layout effects into passive effects so that they did not block paint or stretch event handlers.

Unfortunately this warning currently does not handle a few cases well:

  1. Passive effects might be flushed synchronously (along with their updates) if a layout effect schedules a synchronous update.
  2. Click events now always flush passive effects synchronously (see Bug: useEffect Timing changes depending on if Portal was rendered #20074 (comment)).

This means that the current warning may be confusing or misleading. We should either update it to ensure that it never fires for updates that were already scheduled inside of a passive effect, or if that is not possible we should remove it entirely.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 9, 2021

As a side note, given that React heuristics like this may change between releases (as happened with "click" event behavior) we may need to rethink the source of Timeline warnings. Maybe these warnings belong inside of React itself (DEV and profiling builds) and should be communicated to DevTools using some standardized format?

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 9, 2021

Bit of an update on this:

I created a Code Sandbox to test the current behavior in alpha:
https://codesandbox.io/s/false-positive-long-nested-update-warning-x5zxg?file=/src/App.js:613-618

passive-default.mp4

Both layout and passive effects are processed synchronously, but while layout effect updates are flushed synchronously– passive effect updates are scheduled with "default" priority and flush after paint.

So the 1st scenario described in the issue above is possible, but the 2nd scenario was a misunderstanding between Dan and I.

@bvaughn
Copy link
Contributor Author

bvaughn commented Nov 9, 2021

Here is a code sandbox that does show a false warning though:
https://b75v7.csb.app/

And here is the JSON profile:
Profile-20211109T163441.txt

Not sure exactly what's going on here, but we shouldn't be warning about this case since the update isn't actually being processed synchronously. (It's not blocking paint.)

Screen Shot 2021-11-09 at 4 37 14 PM

@andrelmchaves
Copy link

andrelmchaves commented Apr 17, 2022

@bvaughn Could you confirm this is still an issue?

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

No branches or pull requests

2 participants