From ffbb86b361e5cfec0c3c5dcc2f5ab829e7f54527 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 28 Dec 2016 20:41:40 -0800 Subject: [PATCH 1/6] Wrap top-level mount and unmount in syncUpdates For legacy purposes. Only enabled in the DOM renderer. We can remove this in a future release when we enable incremental-by-default. This change is unobservable because syncUpdates actually schedules Task updates when it is called from inside another batch. The correct behavior is to recursively begin another batch of work. We will fix it in a subsequent commit. --- src/renderers/dom/fiber/ReactDOMFiber.js | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index 20d851dcee..a4ed6d3e54 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -322,9 +322,15 @@ function renderSubtreeIntoContainer(parentComponent : ?ReactComponent { + DOMRenderer.updateContainer(children, newRoot, parentComponent, callback); + }); + } else { + DOMRenderer.updateContainer(children, root, parentComponent, callback); } - DOMRenderer.updateContainer(children, root, parentComponent, callback); return DOMRenderer.getPublicRootInstance(root); } @@ -346,8 +352,11 @@ var ReactDOM = { unmountComponentAtNode(container : DOMContainerElement) { warnAboutUnstableUse(); if (container._reactRootContainer) { - return renderSubtreeIntoContainer(null, null, container, () => { - container._reactRootContainer = null; + // Unmount is always sync, even if we're in a batch. + return DOMRenderer.syncUpdates(() => { + return renderSubtreeIntoContainer(null, null, container, () => { + container._reactRootContainer = null; + }); }); } }, From 3926a3566fc8c28758b9f816006ea2495c63c71d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 28 Dec 2016 20:51:42 -0800 Subject: [PATCH 2/6] If an error is thrown and there's no nextUnitOfWork, it's fatal error This can only be caused by a bug in the renderer, but we should handle it gracefully anyway. Added a TODO to change capturedErrors and failedBoundaries so that they are sets of instances (stateNodes) rather than fibers, to avoid having to check the alternate. (This outside the scope of this PR.) --- .../shared/fiber/ReactFiberScheduler.js | 62 ++++++++++--------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 63cea47ee4..bc14bf3693 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -705,39 +705,40 @@ module.exports = function(config : HostConfig(config : HostConfig(config : HostConfig Date: Wed, 28 Dec 2016 21:02:50 -0800 Subject: [PATCH 3/6] Allow performWork to be called recursively Currently we assume that performWork is never called recursively. Ideally that is the case. We shouldn't rely on recursion anywhere in Fiber. However, to support the use of syncUpdates as an opt-in to forced- synchronous updates, we need the ability to call performWork recursively. At the beginning of performWork, the state of the scheduler is saved; before exiting, that state is restored, so that the scheduler can continue where it left off. I've decided to use local variables to stash the previous state. We could also store them in a record and push it to a stack, but this approach has the advantage of being isolated to performWork. --- .../shared/fiber/ReactFiberScheduler.js | 55 +++++++++++++------ 1 file changed, 37 insertions(+), 18 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index bc14bf3693..b4a8aeb7e6 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -672,10 +672,29 @@ module.exports = function(config : HostConfig(config : HostConfig Date: Wed, 28 Dec 2016 21:12:34 -0800 Subject: [PATCH 4/6] syncUpdates prevents Synchronous priority from being downgraded to Task Typically, a sync update is downgraded to Task priority if it's scheduled within another batch of work, e.g. within a lifecycle like componentDidMount. But syncUpdates should force sync updates to not be downgraded to Task. This will cause performWork to be called recursively. --- .../shared/fiber/ReactFiberScheduler.js | 34 +++++++++++-------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index b4a8aeb7e6..45fc8dbf36 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -107,10 +107,12 @@ module.exports = function(config : HostConfig(config : HostConfig(config : HostConfig(config : HostConfig(config : HostConfig(config : HostConfig(config : HostConfig(fn : (a: A) => R, a : A) : R { - const previousIsPerformingWork = isPerformingWork; - // Simulate that we're performing work so that sync work is batched - isPerformingWork = true; + const previousShouldDeferSyncUpdates = shouldDeferSyncUpdates; + shouldDeferSyncUpdates = true; try { return fn(a); } finally { - isPerformingWork = previousIsPerformingWork; - // If we're not already performing work, we need to flush any task work + shouldDeferSyncUpdates = previousShouldDeferSyncUpdates; + // If we're not already inside a batch, we need to flush any task work // that was created by the user-provided function. - if (!isPerformingWork) { + if (!shouldDeferSyncUpdates) { performWork(TaskPriority); } } @@ -1125,11 +1126,14 @@ module.exports = function(config : HostConfig(fn : () => A) : A { const previousPriorityContext = priorityContext; + const previousShouldDeferSyncUpdates = shouldDeferSyncUpdates; priorityContext = SynchronousPriority; + shouldDeferSyncUpdates = false; try { return fn(); } finally { priorityContext = previousPriorityContext; + shouldDeferSyncUpdates = previousShouldDeferSyncUpdates; } } From ce8c978d6305e22bf27ea9754cc4176a0eabd4ef Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 28 Dec 2016 21:28:40 -0800 Subject: [PATCH 5/6] Disallow forced sync updates during begin phase Stack allows this, but in Fiber it may cause an infinite loop. Instead we'll print a warning and defer the update by giving it Task priority. --- .../shared/__tests__/ReactPerf-test.js | 19 ++++++++++++++-- .../shared/fiber/ReactFiberScheduler.js | 14 ++++++++++-- .../ReactIncrementalScheduling-test.js | 10 +++++++++ .../__tests__/ReactComponentTreeHook-test.js | 13 +++++++++++ .../__tests__/ReactCompositeComponent-test.js | 2 +- .../shared/__tests__/ReactUpdates-test.js | 22 +++++++++++++++++++ 6 files changed, 75 insertions(+), 5 deletions(-) diff --git a/src/renderers/shared/__tests__/ReactPerf-test.js b/src/renderers/shared/__tests__/ReactPerf-test.js index b8f895fc88..2695e648b2 100644 --- a/src/renderers/shared/__tests__/ReactPerf-test.js +++ b/src/renderers/shared/__tests__/ReactPerf-test.js @@ -16,6 +16,7 @@ describe('ReactPerf', () => { var ReactDOM; var ReactPerf; var ReactTestUtils; + var ReactDOMFeatureFlags; var emptyFunction; var App; @@ -38,6 +39,7 @@ describe('ReactPerf', () => { ReactDOM = require('ReactDOM'); ReactPerf = require('ReactPerf'); ReactTestUtils = require('ReactTestUtils'); + ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); emptyFunction = require('emptyFunction'); App = class extends React.Component { @@ -614,7 +616,7 @@ describe('ReactPerf', () => { } } class EvilPortal extends React.Component { - componentWillMount() { + componentDidMount() { var portalContainer = document.createElement('div'); ReactDOM.render(, portalContainer); } @@ -645,6 +647,10 @@ describe('ReactPerf', () => { var container = document.createElement('div'); var thrownErr = new Error('Muhaha!'); + if (ReactDOMFeatureFlags.useFiber) { + spyOn(console, 'error'); + } + class Evil extends React.Component { componentWillMount() { throw thrownErr; @@ -679,6 +685,15 @@ describe('ReactPerf', () => { } ReactDOM.unmountComponentAtNode(container); ReactPerf.stop(); + + if (ReactDOMFeatureFlags.useFiber) { + // A sync `render` inside cWM will print a warning. That should be the + // only warning. + expect(console.error.calls.count()).toEqual(1); + expect(console.error.calls.argsFor(0)[0]).toMatch( + /Render methods should be a pure function of props and state/ + ); + } }); it('should not print errant warnings if portal throws in componentDidMount()', () => { @@ -694,7 +709,7 @@ describe('ReactPerf', () => { } } class EvilPortal extends React.Component { - componentWillMount() { + componentDidMount() { var portalContainer = document.createElement('div'); ReactDOM.render(, portalContainer); } diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index 45fc8dbf36..e78d1c40ee 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -69,6 +69,7 @@ var { if (__DEV__) { var ReactFiberInstrumentation = require('ReactFiberInstrumentation'); var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); + var warning = require('warning'); } var timeHeuristicForUnitOfWork = 1; @@ -1029,8 +1030,17 @@ module.exports = function(config : HostConfig { // animation priority. expect(ReactNoop.getChildren()).toEqual([span(1)]); }); + + it('can force synchronous updates with syncUpdates, even inside batchedUpdates', done => { + ReactNoop.batchedUpdates(() => { + ReactNoop.syncUpdates(() => { + ReactNoop.render(); + expect(ReactNoop.getChildren()).toEqual([span()]); + done(); + }); + }); + }); }); diff --git a/src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js b/src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js index db281517a4..e0114b5643 100644 --- a/src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js +++ b/src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js @@ -18,6 +18,7 @@ describe('ReactComponentTreeHook', () => { var ReactInstanceMap; var ReactComponentTreeHook; var ReactComponentTreeTestUtils; + var ReactDOMFeatureFlags; beforeEach(() => { jest.resetModules(); @@ -28,6 +29,7 @@ describe('ReactComponentTreeHook', () => { ReactInstanceMap = require('ReactInstanceMap'); ReactComponentTreeHook = require('ReactComponentTreeHook'); ReactComponentTreeTestUtils = require('ReactComponentTreeTestUtils'); + ReactDOMFeatureFlags = require('ReactDOMFeatureFlags'); }); function assertTreeMatches(pairs) { @@ -1841,6 +1843,9 @@ describe('ReactComponentTreeHook', () => { // https://github.com/facebook/react/issues/7187 var el = document.createElement('div'); var portalEl = document.createElement('div'); + if (ReactDOMFeatureFlags.useFiber) { + spyOn(console, 'error'); + } class Foo extends React.Component { componentWillMount() { ReactDOM.render(
, portalEl); @@ -1850,6 +1855,14 @@ describe('ReactComponentTreeHook', () => { } } ReactDOM.render(, el); + if (ReactDOMFeatureFlags.useFiber) { + // A sync `render` inside cWM will print a warning. That should be the + // only warning. + expect(console.error.calls.count()).toEqual(1); + expect(console.error.calls.argsFor(0)[0]).toMatch( + /Render methods should be a pure function of props and state/ + ); + } }); it('is created when calling renderToString during render', () => { diff --git a/src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js b/src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js index 1a302d6fca..af226d2f22 100644 --- a/src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js +++ b/src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js @@ -1266,7 +1266,7 @@ describe('ReactCompositeComponent', () => { var layer = document.createElement('div'); class Component extends React.Component { - componentWillMount() { + componentDidMount() { ReactDOM.render(
, layer); } diff --git a/src/renderers/shared/shared/__tests__/ReactUpdates-test.js b/src/renderers/shared/shared/__tests__/ReactUpdates-test.js index 82113a46d3..638ab6eda2 100644 --- a/src/renderers/shared/shared/__tests__/ReactUpdates-test.js +++ b/src/renderers/shared/shared/__tests__/ReactUpdates-test.js @@ -1142,4 +1142,26 @@ describe('ReactUpdates', () => { expect(container.textContent).toBe('goodbye'); expect(mounts).toBe(1); }); + + it('mounts and unmounts are sync even in a batch', () => { + var container1 = document.createElement('div'); + var container2 = document.createElement('div'); + + let called = false; + class Foo extends React.Component { + componentDidMount() { + called = true; + ReactDOM.render(
Hello
, container2); + expect(container2.textContent).toEqual('Hello'); + ReactDOM.unmountComponentAtNode(container2); + expect(container2.textContent).toEqual(''); + } + render() { + return
{this.props.step}
; + } + } + + ReactDOM.render(, container1); + expect(called).toEqual(true); + }); }); From 79f01b242531bc3706c7f254871eb37e43f60e85 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 28 Dec 2016 21:57:34 -0800 Subject: [PATCH 6/6] prepareForCommit returns info object to be passed resetAfterCommit The DOM renderer assumes that resetAfterCommit is called after prepareForCommit without any nested commits in between. That may not be the case now that syncUpdates forces a nested update. To address, this changes the type of prepareForCommit to return a value which is later passed to resetAfterCommit. --- scripts/fiber/tests-passing.txt | 2 ++ src/renderers/dom/fiber/ReactDOMFiber.js | 24 ++++++++++--------- .../shared/fiber/ReactFiberBeginWork.js | 4 ++-- .../shared/fiber/ReactFiberCommitWork.js | 4 ++-- .../shared/fiber/ReactFiberCompleteWork.js | 4 ++-- .../shared/fiber/ReactFiberHostContext.js | 4 ++-- .../shared/fiber/ReactFiberReconciler.js | 8 +++---- .../shared/fiber/ReactFiberScheduler.js | 6 ++--- 8 files changed, 30 insertions(+), 26 deletions(-) diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index c56a44b5b2..c749ed37ab 100644 --- a/scripts/fiber/tests-passing.txt +++ b/scripts/fiber/tests-passing.txt @@ -1207,6 +1207,7 @@ src/renderers/shared/fiber/__tests__/ReactIncrementalScheduling-test.js * can opt-in to deferred/animation scheduling inside componentDidMount/Update * performs Task work even after time runs out * does not perform animation work after time runs out +* can force synchronous updates with syncUpdates, even inside batchedUpdates src/renderers/shared/fiber/__tests__/ReactIncrementalSideEffects-test.js * can update child nodes of a host instance @@ -1576,6 +1577,7 @@ src/renderers/shared/shared/__tests__/ReactUpdates-test.js * unstable_batchedUpdates should return value from a callback * unmounts and remounts a root in the same batch * handles reentrant mounting in synchronous mode +* mounts and unmounts are sync even in a batch src/renderers/shared/shared/__tests__/refs-destruction-test.js * should remove refs when destroying the parent diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index a4ed6d3e54..26c3734909 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -67,9 +67,10 @@ type HostContextDev = { }; type HostContextProd = string; type HostContext = HostContextDev | HostContextProd; - -let eventsEnabled : ?boolean = null; -let selectionInformation : ?mixed = null; +type CommitInfo = { + eventsEnabled: boolean, + selectionInformation: mixed, +}; var ELEMENT_NODE_TYPE = 1; var DOC_NODE_TYPE = 9; @@ -137,17 +138,18 @@ var DOMRenderer = ReactFiberReconciler({ return getChildNamespace(parentNamespace, type); }, - prepareForCommit() : void { - eventsEnabled = ReactBrowserEventEmitter.isEnabled(); + prepareForCommit() : CommitInfo { + const eventsEnabled = ReactBrowserEventEmitter.isEnabled(); ReactBrowserEventEmitter.setEnabled(false); - selectionInformation = ReactInputSelection.getSelectionInformation(); + return { + eventsEnabled, + selectionInformation: ReactInputSelection.getSelectionInformation(), + }; }, - resetAfterCommit() : void { - ReactInputSelection.restoreSelection(selectionInformation); - selectionInformation = null; - ReactBrowserEventEmitter.setEnabled(eventsEnabled); - eventsEnabled = null; + resetAfterCommit(commitInfo : CommitInfo) : void { + ReactInputSelection.restoreSelection(commitInfo.selectionInformation); + ReactBrowserEventEmitter.setEnabled(commitInfo.eventsEnabled); }, createInstance( diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index 4cfb1f4bb1..e5ee7a831d 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -66,8 +66,8 @@ if (__DEV__) { var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); } -module.exports = function( - config : HostConfig, +module.exports = function( + config : HostConfig, hostContext : HostContext, scheduleUpdate : (fiber : Fiber, priorityLevel : PriorityLevel) => void, getPriorityContext : () => PriorityLevel, diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index bb56a4c78f..429e892e0b 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -33,8 +33,8 @@ var { ContentReset, } = require('ReactTypeOfSideEffect'); -module.exports = function( - config : HostConfig, +module.exports = function( + config : HostConfig, hostContext : HostContext, captureError : (failedFiber : Fiber, error: Error) => ?Fiber ) { diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index bffecb3c46..8c836cfda5 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -46,8 +46,8 @@ if (__DEV__) { var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); } -module.exports = function( - config : HostConfig, +module.exports = function( + config : HostConfig, hostContext : HostContext, ) { const { diff --git a/src/renderers/shared/fiber/ReactFiberHostContext.js b/src/renderers/shared/fiber/ReactFiberHostContext.js index 529271de6a..de311124ca 100644 --- a/src/renderers/shared/fiber/ReactFiberHostContext.js +++ b/src/renderers/shared/fiber/ReactFiberHostContext.js @@ -34,8 +34,8 @@ export type HostContext = { resetHostContainer() : void, }; -module.exports = function( - config : HostConfig +module.exports = function( + config : HostConfig ) : HostContext { const { getChildHostContext, diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index ff55cbddaa..3424ef683f 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -43,7 +43,7 @@ export type Deadline = { type OpaqueNode = Fiber; -export type HostConfig = { +export type HostConfig = { getRootHostContext(rootContainerInstance : C) : CX, getChildHostContext(parentHostContext : CX, type : T) : CX, @@ -69,8 +69,8 @@ export type HostConfig = { scheduleAnimationCallback(callback : () => void) : void, scheduleDeferredCallback(callback : (deadline : Deadline) => void) : void, - prepareForCommit() : void, - resetAfterCommit() : void, + prepareForCommit() : CI, + resetAfterCommit(commitInfo : CI) : void, useSyncScheduling ?: boolean, }; @@ -100,7 +100,7 @@ getContextForSubtree._injectFiber(function(fiber : Fiber) { parentContext; }); -module.exports = function(config : HostConfig) : Reconciler { +module.exports = function(config : HostConfig) : Reconciler { var { scheduleUpdate, diff --git a/src/renderers/shared/fiber/ReactFiberScheduler.js b/src/renderers/shared/fiber/ReactFiberScheduler.js index e78d1c40ee..20603ec688 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -74,7 +74,7 @@ if (__DEV__) { var timeHeuristicForUnitOfWork = 1; -module.exports = function(config : HostConfig) { +module.exports = function(config : HostConfig) { const hostContext = ReactFiberHostContext(config); const { popHostContainer, popHostContext, resetHostContainer } = hostContext; const { beginWork, beginFailedWork } = ReactFiberBeginWork( @@ -348,7 +348,7 @@ module.exports = function(config : HostConfig(config : HostConfig