Schedule state and callback at the same time #8585

acdlite · 2016-12-15T23:55:22Z

Fixes an issue (in both Stack and Fiber) where enqueueSetState causes a synchronous update that flushes before enqueueCallback is ever called. Now enqueueSetState et al accept an optional callback so that both are scheduled at the same time.

bvaughn · 2016-12-20T21:21:37Z

src/renderers/shared/fiber/ReactFiberUpdateQueue.js

+  const isTopLevelUnmount = Boolean(
+    partialState &&
+    partialState.element === null
+  );


At a glance, this change seems unrelated to the core of this PR, and also like the opposite of what the logic used to be. Was it a bug before? I don't see any other change inside of this PR that looks as though it would have invalidated the previous check, but maybe I'm missing it.

eg I would expect to see a change more like

const isTopLevelUnmount = ( partialState === null || partialState.element === null );

partialState at the top level is always an object with the shape { element: something }. The previous test was just wrong. I thought about writing a unit test for this but it's unobservable except in incremental mode. And when we do switch to incremental by default the top-level mount/update/unmount API will have to change anyway.

Your suggested change is wrong because

Object.assign({}, { element: element }, null);

merely copies the previous state, whereas an unmount sets element to null.

The previous test was just wrong.

Gotcha. This is what I thought may be the case but I wasn't sure.

bvaughn · 2016-12-20T21:27:39Z

src/renderers/shared/stack/reconciler/ReactUpdateQueue.js

+      } else {
+        internalInstance._pendingCallbacks = [callback];
+      }
+    }


Do we manually inline these 3 blocks (and the one in enqueueCallback) for performance reasons?

Didn't seem worth extracting into a function.

sophiebits · 2016-12-20T22:18:08Z

src/renderers/shared/shared/__tests__/ReactCompositeComponentState-test.js

-    );
-
-    if (ReactDOMFeatureFlags.useFiber) {
-      expected.push(['initial-callback', 'yellow']);


What happened here?

Explained by the now-deleted comment above:

In Fiber, the initial callback is not enqueued until after any work scheduled by lifecycles has flushed (same semantics as a regular setState callback outside of a batch). In Stack, the initial render is scheduled inside of batchedUpdates, so the callback gets flushed right after componentDidMount.

TODO: We should fix this, in both Stack and Fiber, so that the behavior is consistent regardless of whether you're in a batch.

sophiebits · 2016-12-20T22:26:03Z

src/renderers/shared/shared/__tests__/ReactUpdates-test.js

          'Inner-setState-1',
-            'Inner-render-1-1',


Why did this rerender immediately before? I thought this change would only move some setState callbacks earlier.

I'm actually not sure why it re-rendered immediately before, since setState is called inside componentDidUpdate which should happen in a batch regardless. Is this too significant a change to make in a non-major release?

We should figure out why there was a change before we can make a meaningful decision on that point. We've made other breaking changes for Fiber so we'll need to do a 16 regardless before releasing Fiber (or maybe at the same time).

sophiebits · 2016-12-21T01:28:55Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

      const fiber = ReactInstanceMap.get(instance);
-      scheduleSetState(fiber, partialState);
+      const priorityLevel = getPriorityContext();


Why did you choose to split these out rather than adding callback as an argument to scheduleSetState etc? This feels like it's asking for someone to accidentally introduce the problem this PR is fixing.

I was going to remove scheduleSetState et al from the scheduler anyway because they were just an extra layer of indirection around addUpdate et al. See @sebmarkbage's comment here for context: #8538 (comment)

To avoid introducing this problem again we could delete addCallback and enqueueCallback since they're no longer used. Sebastian thought it was better not to break the existing class updater API on the off chance someone was relying on it.

I'd rather remove them. I don't think we've ever described these as public at all. Google doesn't show anyone talking about them. I'll leave it up to you though.

Ok then I'll remove

sophiebits · 2016-12-21T18:48:35Z

src/renderers/shared/fiber/ReactFiberClassComponent.js

      const fiber = ReactInstanceMap.get(instance);
-      scheduleSetState(fiber, partialState);
+      const priorityLevel = getPriorityContext();


I'd rather remove them. I don't think we've ever described these as public at all. Google doesn't show anyone talking about them. I'll leave it up to you though.

Fixes an issue (in both Stack and Fiber) where enqueueSetState causes a synchronous update that flushes before enqueueCallback is ever called. Now enqueueSetState et al accept an optional callback so that both are scheduled at the same time.

This isn't being used anymore. Also removes addCallback from ReactFiberUpdateQueue.

facebook-github-bot added GH Review: review-needed CLA Signed labels Dec 15, 2016

acdlite mentioned this pull request Dec 16, 2016

[Fiber] Queue top-level updates #8584

Merged

acdlite force-pushed the batchstateandcallback branch 2 times, most recently from d99319c to 9f68a27 Compare December 20, 2016 21:08

acdlite changed the title ~~[WIP] Schedule state and callback at the same time~~ Schedule state and callback at the same time Dec 20, 2016

acdlite requested review from sophiebits, bvaughn, sebmarkbage and gaearon December 20, 2016 21:09

bvaughn reviewed Dec 20, 2016

View reviewed changes

sophiebits reviewed Dec 20, 2016

View reviewed changes

sophiebits reviewed Dec 21, 2016

View reviewed changes

sophiebits approved these changes Dec 21, 2016

View reviewed changes

acdlite added 2 commits December 21, 2016 12:54

Remove enqueueCallback from updater API

f4c0c99

This isn't being used anymore. Also removes addCallback from ReactFiberUpdateQueue.

acdlite force-pushed the batchstateandcallback branch from 9f68a27 to f4c0c99 Compare December 21, 2016 19:03

acdlite merged commit d47a3a3 into facebook:master Dec 21, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Schedule state and callback at the same time #8585

Schedule state and callback at the same time #8585

acdlite commented Dec 15, 2016 •

edited

Loading

bvaughn Dec 20, 2016 •

edited

Loading

acdlite Dec 20, 2016

acdlite Dec 20, 2016

bvaughn Dec 20, 2016

bvaughn Dec 20, 2016

acdlite Dec 20, 2016

sophiebits Dec 20, 2016

acdlite Dec 20, 2016

sophiebits Dec 20, 2016

acdlite Dec 20, 2016

sophiebits Dec 20, 2016

sophiebits Dec 21, 2016

acdlite Dec 21, 2016 •

edited

Loading

sophiebits Dec 21, 2016

acdlite Dec 21, 2016

sophiebits Dec 21, 2016

Schedule state and callback at the same time #8585

Schedule state and callback at the same time #8585

Conversation

acdlite commented Dec 15, 2016 • edited Loading

bvaughn Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Dec 15, 2016 •

edited

Loading

bvaughn Dec 20, 2016 •

edited

Loading

acdlite Dec 21, 2016 •

edited

Loading