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

[Fiber] setState #7344

Merged
merged 15 commits into from Sep 13, 2016

Conversation

Projects
None yet
5 participants
@acdlite
Member

acdlite commented Jul 24, 2016

workInProgress (yay puns)

Implements setState for Fiber. Updates are scheduled by setting a work priority on the fiber and bubbling it to the root. Because the instance does not know which tree is current at any given time, the update is scheduled on both fiber alternates.

Need to add more unit tests to cover edge cases.

A few questions I have:

  • How to deal with the circular dependency between ReactFiberScheduler and ReactFiberBeginWork. I'm using a closure to lazily access the scheduler but seems like there may be a better way.
  • How to queue update callbacks. The only way I could think to do it was to add another field to Fiber, so I've punted on this for now.
  • How to access the fiber from the updater. I decided to use a private field on the instance. Could also create a new updater for each instance and close over its fiber, but that seems wasteful.
  • Should I put setState related tests in their own module? I think I should but not sure.

(I discussed with @sebmarkbage before working on this PR.)

Based on #7448. Compare view: sebmarkbage/react@fiberstarvation...acdlite:fibersetstate

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jul 24, 2016

Member

We'll figure something out with the dependency later. One technique is to just put everything in the same file and then try to break it apart again into reasonable pieces.

I figured that the simple pendingState could an object, and then if you use setState((state, props) => ...); or the callback model or it would turn into an array or linked list.

To access the fiber from the updater, you can use a field. That's what we do now. However, you could switch the implementation to use ReactInstanceMap which the current implementation does so we can share that code. Just move it to: src/renderers/shared/shared/ to indicate that we would like to share it between both implementations.

Member

sebmarkbage commented Jul 24, 2016

We'll figure something out with the dependency later. One technique is to just put everything in the same file and then try to break it apart again into reasonable pieces.

I figured that the simple pendingState could an object, and then if you use setState((state, props) => ...); or the callback model or it would turn into an array or linked list.

To access the fiber from the updater, you can use a field. That's what we do now. However, you could switch the implementation to use ReactInstanceMap which the current implementation does so we can share that code. Just move it to: src/renderers/shared/shared/ to indicate that we would like to share it between both implementations.

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jul 24, 2016

Member

As for the unit tests, we can keep them in Incremental for now. In the near future we'll just run this against the existing unit tests for setState. We'll evolve the tests you're writing now into tests that specifically tests the new incremental effects of setState.

Member

sebmarkbage commented Jul 24, 2016

As for the unit tests, we can keep them in Incremental for now. In the near future we'll just run this against the existing unit tests for setState. We'll evolve the tests you're writing now into tests that specifically tests the new incremental effects of setState.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jul 24, 2016

Member

Ah keeping a queue of pending states makes sense. A linked list sounds like the way to go, but how to disambiguate between a state object and a linked list? $$typeof?

Member

acdlite commented Jul 24, 2016

Ah keeping a queue of pending states makes sense. A linked list sounds like the way to go, but how to disambiguate between a state object and a linked list? $$typeof?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jul 25, 2016

Member

Always wrapping it in a linked list node might be fine. Should be short lived and not a very hot path.

On Jul 24, 2016, at 4:44 PM, Andrew Clark notifications@github.com wrote:

Ah keeping a queue of pending states makes sense. A linked list sounds like the way to go, but how to disambiguate between a state object and a linked list? $$typeof?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Member

sebmarkbage commented Jul 25, 2016

Always wrapping it in a linked list node might be fine. Should be short lived and not a very hot path.

On Jul 24, 2016, at 4:44 PM, Andrew Clark notifications@github.com wrote:

Ah keeping a queue of pending states makes sense. A linked list sounds like the way to go, but how to disambiguate between a state object and a linked list? $$typeof?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jul 25, 2016

Member

Changed the type of pendingState to be a queue.

Regarding typing, I'm guessing eventually the type of the props and state will be inferred from the element? For now I've used any because that's what props uses, but I know that's bad.

Also not sure about naming. pendingState is confusing because it's a queue of partial states, whereas pendingProps is a single props object.

Member

acdlite commented Jul 25, 2016

Changed the type of pendingState to be a queue.

Regarding typing, I'm guessing eventually the type of the props and state will be inferred from the element? For now I've used any because that's what props uses, but I know that's bad.

Also not sure about naming. pendingState is confusing because it's a queue of partial states, whereas pendingProps is a single props object.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jul 25, 2016

Member

Maybe rename pendingState to stateQueue?

Member

acdlite commented Jul 25, 2016

Maybe rename pendingState to stateQueue?

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Jul 25, 2016

Member

Sure. Wouldn't worry too much about it yet until all the pieces are there and have proven themselves.

It's quite possible that props will become a queue too if we want to solve the problem of props not being consistent when an event happens.

On Jul 24, 2016, at 11:04 PM, Andrew Clark notifications@github.com wrote:

Maybe rename pendingState to stateQueue?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

Member

sebmarkbage commented Jul 25, 2016

Sure. Wouldn't worry too much about it yet until all the pieces are there and have proven themselves.

It's quite possible that props will become a queue too if we want to solve the problem of props not being consistent when an event happens.

On Jul 24, 2016, at 11:04 PM, Andrew Clark notifications@github.com wrote:

Maybe rename pendingState to stateQueue?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.

@ghost ghost added CLA Signed labels Aug 1, 2016

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Aug 2, 2016

Member

A few things left to do.

We need a separate field for the priority of a fiber's pending props and state, distinct from the existing field pendingWorkPriority, which represents the highest priority of the entire subtree. Otherwise, there's no way to schedule work deep in the tree without overriding the priority of every ancestor. I got this working locally but I'm holding off until @sebmarkbage fixes some priority-related bugs on his branch.

We also need more tests, specifically around preempted updates. In order to do this, we need to implement high priority updates. @sebmarkbage's idea is an API like ReactNoop.performHighPriWork(fn) where all updates called within that scope have high priority by default.

Member

acdlite commented Aug 2, 2016

A few things left to do.

We need a separate field for the priority of a fiber's pending props and state, distinct from the existing field pendingWorkPriority, which represents the highest priority of the entire subtree. Otherwise, there's no way to schedule work deep in the tree without overriding the priority of every ancestor. I got this working locally but I'm holding off until @sebmarkbage fixes some priority-related bugs on his branch.

We also need more tests, specifically around preempted updates. In order to do this, we need to implement high priority updates. @sebmarkbage's idea is an API like ReactNoop.performHighPriWork(fn) where all updates called within that scope have high priority by default.

@ghost ghost added the CLA Signed label Aug 2, 2016

@ghost ghost added CLA Signed labels Aug 3, 2016

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite
Member

acdlite commented Aug 6, 2016

@sebmarkbage Rebased

@ghost ghost added the CLA Signed label Aug 6, 2016

@ghost ghost added the CLA Signed label Aug 6, 2016

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Aug 6, 2016

Member

@sebmarkbage Pushed some changes that I believe address your comments. I think I understand the relationship between the current tree and work in progress tree better now.

Member

acdlite commented Aug 6, 2016

@sebmarkbage Pushed some changes that I believe address your comments. I think I understand the relationship between the current tree and work in progress tree better now.

@ghost ghost added the CLA Signed label Sep 6, 2016

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Sep 6, 2016

Coverage Status

Coverage increased (+0.005%) to 87.196% when pulling 319e755 on acdlite:fibersetstate into 6a525fd on facebook:master.

coveralls commented Sep 6, 2016

Coverage Status

Coverage increased (+0.005%) to 87.196% when pulling 319e755 on acdlite:fibersetstate into 6a525fd on facebook:master.

acdlite and others added some commits Jul 24, 2016

setState for Fiber
Updates are scheduled by setting a work priority on the fiber and bubbling it to
the root. Because the instance does not know which tree is current at any given
time, the update is scheduled on both fiber alternates.

Need to add more unit tests to cover edge cases.
Use queue for pendingState
Changes the type of pendingState to be a linked list of partial
state objects.
Updater form of setState
Add support for setState((state, props) => newState).

Rename pendingState to stateQueue.
Clean up
Rather than bubble up both trees, bubble up once and assign to the
alternate at each level.

Extract logic for adding to the queue to the StateQueue module.
Clean up
Use a union type for the head of StateQueue.
Use ReactInstanceMap
Move ReactInstanceMap to src/renderers/shared/shared to indicate that
this logic is shared across implementations.
Rename stateQueue -> updateQueue
Also cleans up some types.
Update callbacks
Callbacks are stored on the same queue as updates. They care called
during the commit phase, after the updates have been flushed.

Because the update queue is cleared during the completion phase (before
commit), a new field has been added to fiber called callbackList. The
queue is transferred from updateQueue to callbackList during completion.
During commit, the list is reset.

Need a test to confirm that callbacks are not lost if an update is
preempted.
replaceState
Adds a field to UpdateQueue that indicates whether an update should
replace the previous state completely.
forceUpdate
Adds a field to the update queue that causes shouldComponentUpdate to
be skipped.
Don't mutate current tree before work is committed.
We should be able to abort an update without any side-effects to the
current tree. This fixes a few cases where that was broken.

The callback list should only ever be set on the workInProgress.
There's no reason to add it to the current tree because they're not
needed after they are called during the commit phase.

Also found a bug where the memoizedProps were set to null in the
case of an update, because the pendingProps were null. Fixed by
transfering the props from the instance, like we were already doing
with state.

Added a test to ensure that setState can be called inside a
callback.
Check for truthiness of alternate
This is unfortunate since we agreed on using the `null | Fiber`
convention instead of `?Fiber` but haven't upgraded yet and this
is the pattern I've been using everywhere else so far.
Log the updateQueue in dumpTree
This also buffers all rows into a single console.log call.
This is because jest nows adds the line number of each console.log
call which becomes quite noisy for these trees.

@ghost ghost added the CLA Signed label Sep 13, 2016

@sebmarkbage

This comment has been minimized.

Show comment
Hide comment
@sebmarkbage

sebmarkbage Sep 13, 2016

Member

Ninja merging this. Will fix travis if errors are caused. #workedonmymachine

Member

sebmarkbage commented Sep 13, 2016

Ninja merging this. Will fix travis if errors are caused. #workedonmymachine

@sebmarkbage sebmarkbage merged commit 3e54b28 into facebook:master Sep 13, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@zpao zpao modified the milestone: 15-next Sep 19, 2016

@zpao zpao modified the milestones: 15-next, 15.4.0 Oct 4, 2016

zpao added a commit that referenced this pull request Oct 4, 2016

Merge pull request #7344 from acdlite/fibersetstate
[Fiber] setState
(cherry picked from commit 3e54b28)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment