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

[Fiber] UpdateQueue follow-up improvements #8587

Merged
merged 4 commits into from
Dec 17, 2016
Merged

[Fiber] UpdateQueue follow-up improvements #8587

merged 4 commits into from
Dec 17, 2016

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Dec 16, 2016 •
edited
Loading

Follow-up to #8538.

  • Replace console.error with warning module.
  • Remove ForceUpdate effect and put the flag back on the queue.
  • Don't need to track isEmpty
  • Don't drop updates from the queue in replaceState and always pass the accumulated state.
  • Inline insertUpdateIntoQueue, maybe. Make the logic easier to follow when an incoming update is cloned versus when it is not.
  • Don't update memoizedState in begin phase. Later, we'll move all memoization from complete to begin.
  • Get rid of weird for-loops. Either explode or use a helper function so it can be inlined.
  • Clone update before transferring to callbackList

@acdlite
Clean up some style nits and other code detritus in the update queue
dfe3d08
@acdlite
Remove confusing for-loop in favor of a helper function
f070aaa 
This logic is hopefully easier to follow. Added more inline comments
to explain how the algorithm works.
sebmarkbage
sebmarkbage approved these changes Dec 16, 2016
View reviewed changes
Copy link
Collaborator

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Accepted pending the unit test changes. We shouldn't land this removal.

@acdlite
Don't drop updates from queue when calling replaceState
7b6c94c 
We should be able to support passing a function to replaceState, which
receives the accumulation of all the previously applied state updates.
Which means we shouldn't drop those updates from the queue. Technically,
we could drop them only when an object is passed to replaceState, but
that seems like more trouble than it's worth.
@acdlite acdlite merged commit e1eccbf into facebook:master Dec 17, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sebmarkbage sebmarkbage sebmarkbage approved these changes

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@acdlite @sebmarkbage @facebook-github-bot