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

Fix ignored sync work in passive effects #14799

Merged
merged 2 commits into from
Feb 12, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Feb 8, 2019

Attempts to fix #14792. I don't really know this code but this is my guess.

Specifically, this code doesn't always end up being called from a root work cycle. It can run from the scheduler (indeed, that's the normal case). In that case isRendering is true inside (so ReactDOM.render doesn't do anything) but we don't flush anything before the exit. Now we do.

This delays the actual render until the end of calling effects. I think it makes sense as it's consistent with what we do for commit phase effects.

@sebmarkbage
Copy link
Collaborator

I'll let @acdlite review this one. Whenever he sees a bug like this, he finds two others.

@gaearon
Copy link
Collaborator Author

gaearon commented Feb 11, 2019

Fixed batching, good repro @sophiebits

@acdlite
Copy link
Collaborator

acdlite commented Feb 11, 2019

We do the same thing at the end of batchedUpdates et al. It's annoying, but will hopefully go away once we rely more on the Scheduler package (since Scheduler already does this check at the end of every task).

@gaearon gaearon merged commit 3ae94e1 into facebook:master Feb 12, 2019
This was referenced Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Question: ReactDOM render call in useEffect delayed until first update
5 participants