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.

Sign up for GitHub

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

Jump to bottom

Fixes #8012 where a race condition with useEffect was causing stale renders of values. #8022

Merged
merged 4 commits into from
Apr 23, 2021
Merged

Fixes #8012 where a race condition with useEffect was causing stale renders of values. #8022

merged 4 commits into from
Apr 23, 2021

Conversation

brainkim
Copy link
Contributor

@brainkim brainkim commented Apr 22, 2021 •
edited
Loading

Fixes
#6699 (maybe)
#7694
#7896
#8012

A race condition with useEffect was causing stale renders of values when reactivevars were called multiple times synchronously.

@brainkim brainkim requested review from benjamn and jcreighton and removed request for benjamn April 22, 2021 19:59
@brainkim brainkim changed the title Brian 8012 fix Fixes #8012 where a race condition with useEffect was causing stale renders of values. Apr 22, 2021
@brainkim brainkim marked this pull request as draft April 22, 2021 20:05
@brainkim
Copy link
Contributor Author

brainkim commented Apr 22, 2021 •
edited
Loading

Hang on it wasn’t the useLayoutEffect that did it. never mind it was I just didn’t inline consumeAndIterate() right

@brainkim brainkim marked this pull request as ready for review April 22, 2021 20:30
benjamn
benjamn reviewed Apr 22, 2021
View reviewed changes
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think useLayoutEffect is an improvement, but I wonder if we can eliminate the onNextChange listening gap completely? Thoughts below.

src/cache/inmemory/reactiveVars.ts Show resolved Hide resolved
src/react/hooks/useReactiveVar.ts Show resolved Hide resolved
src/cache/inmemory/reactiveVars.ts Show resolved Hide resolved
src/react/hooks/__tests__/useReactiveVar.test.tsx Show resolved Hide resolved
benjamn
benjamn approved these changes Apr 23, 2021
View reviewed changes
Copy link
Member

@benjamn benjamn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for adding that <StrictMode> test.

@brainkim brainkim merged commit a7393c6 into main Apr 23, 2021
@brainkim brainkim deleted the brian-8012-fix branch April 23, 2021 19:14
This was referenced Apr 23, 2021
Closed
Closed
Closed
This was referenced May 4, 2021
Closed
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jcreighton jcreighton jcreighton left review comments

@benjamn benjamn benjamn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@brainkim @benjamn @jcreighton