From c6d96db12c2fb41f0e4106328a762f2ea97c5c83 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 19 Dec 2016 16:36:09 -0800 Subject: [PATCH 01/11] Allow renderers to return an update payload in prepareUpdate This then gets stored on updateQueue so that the renderer doesn't need to think about how to store this. It then gets passed into commitUpdate during the commit phase. This allows renderers to do the diffing during the time sliced path, allocate a queue for changes and do only the absolute minimal work to apply those changes in the commit phase. If refs update we still schedule and update. --- src/renderers/art/ReactARTFiber.js | 6 ++++-- src/renderers/dom/fiber/ReactDOMFiber.js | 9 ++++++--- src/renderers/native/ReactNativeFiber.js | 18 +++++++++-------- src/renderers/noop/ReactNoop.js | 8 +++++--- .../shared/fiber/ReactFiberBeginWork.js | 4 ++-- .../shared/fiber/ReactFiberCommitWork.js | 17 ++++++++-------- .../shared/fiber/ReactFiberCompleteWork.js | 20 ++++++++++++------- .../shared/fiber/ReactFiberHostContext.js | 4 ++-- .../shared/fiber/ReactFiberReconciler.js | 8 ++++---- .../shared/fiber/ReactFiberScheduler.js | 4 ++-- 10 files changed, 56 insertions(+), 42 deletions(-) diff --git a/src/renderers/art/ReactARTFiber.js b/src/renderers/art/ReactARTFiber.js index ee9aa7d85915..fcdcc957e0f8 100644 --- a/src/renderers/art/ReactARTFiber.js +++ b/src/renderers/art/ReactARTFiber.js @@ -42,6 +42,8 @@ const TYPES = { TEXT: 'Text', }; +const UPDATE_SIGNAL = {}; + /** Helper Methods */ function addEventListeners(instance, type, listener) { @@ -418,7 +420,7 @@ const ARTRenderer = ReactFiberReconciler({ // Noop }, - commitUpdate(instance, type, oldProps, newProps) { + commitUpdate(instance, updatePayload, type, oldProps, newProps) { instance._applyProps(instance, newProps, oldProps); }, @@ -482,7 +484,7 @@ const ARTRenderer = ReactFiberReconciler({ }, prepareUpdate(domElement, type, oldProps, newProps) { - return true; + return UPDATE_SIGNAL; }, removeChild(parentInstance, child) { diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index fc7cb30e83a9..cc7a8bb54741 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -206,8 +206,9 @@ var DOMRenderer = ReactFiberReconciler({ type : string, oldProps : Props, newProps : Props, + rootContainerInstance : Container, hostContext : HostContext, - ) : boolean { + ) : null | Object { if (__DEV__) { const hostContextDev = ((hostContext : any) : HostContextDev); if (typeof newProps.children !== typeof oldProps.children && ( @@ -218,7 +219,7 @@ var DOMRenderer = ReactFiberReconciler({ validateDOMNesting(null, String(newProps.children), null, ownAncestorInfo); } } - return true; + return {}; }, commitMount( @@ -233,14 +234,16 @@ var DOMRenderer = ReactFiberReconciler({ commitUpdate( domElement : Instance, + updatePayload : Object, type : string, oldProps : Props, newProps : Props, - rootContainerInstance : Container, internalInstanceHandle : Object, ) : void { // Update the internal instance handle so that we know which props are // the current ones. + // TODO: Fix this hack. + var rootContainerInstance = (domElement : any); precacheFiberNode(internalInstanceHandle, domElement); updateProperties(domElement, type, oldProps, newProps, rootContainerInstance); }, diff --git a/src/renderers/native/ReactNativeFiber.js b/src/renderers/native/ReactNativeFiber.js index e95f3b2561e3..a8a77cfee451 100644 --- a/src/renderers/native/ReactNativeFiber.js +++ b/src/renderers/native/ReactNativeFiber.js @@ -122,10 +122,10 @@ const NativeRenderer = ReactFiberReconciler({ commitUpdate( instance : Instance, + updatePayloadTODO : Object, type : string, oldProps : Props, newProps : Props, - rootContainerInstance : Object, internalInstanceHandle : Object ) : void { const viewConfig = instance.viewConfig; @@ -149,7 +149,7 @@ const NativeRenderer = ReactFiberReconciler({ type : string, props : Props, rootContainerInstance : Container, - hostContext : Object, + hostContext : {||}, internalInstanceHandle : Object ) : Instance { const tag = ReactNativeTagHandles.allocateTag(); @@ -185,7 +185,7 @@ const NativeRenderer = ReactFiberReconciler({ createTextInstance( text : string, rootContainerInstance : Container, - hostContext : Object, + hostContext : {||}, internalInstanceHandle : Object, ) : TextInstance { const tag = ReactNativeTagHandles.allocateTag(); @@ -224,11 +224,11 @@ const NativeRenderer = ReactFiberReconciler({ return false; }, - getRootHostContext() { + getRootHostContext() : {||} { return emptyObject; }, - getChildHostContext() { + getChildHostContext() : {||} { return emptyObject; }, @@ -290,9 +290,11 @@ const NativeRenderer = ReactFiberReconciler({ instance : Instance, type : string, oldProps : Props, - newProps : Props - ) : boolean { - return true; + newProps : Props, + rootContainerInstance : Container, + hostContext : {||} + ) : null | Object { + return emptyObject; }, removeChild( diff --git a/src/renderers/noop/ReactNoop.js b/src/renderers/noop/ReactNoop.js index dedf20db6e5b..a34ac57251fe 100644 --- a/src/renderers/noop/ReactNoop.js +++ b/src/renderers/noop/ReactNoop.js @@ -29,6 +29,8 @@ var { } = require('ReactPriorityLevel'); var emptyObject = require('emptyObject'); +const UPDATE_SIGNAL = {}; + var scheduledAnimationCallback = null; var scheduledDeferredCallback = null; @@ -78,15 +80,15 @@ var NoopRenderer = ReactFiberReconciler({ return false; }, - prepareUpdate(instance : Instance, type : string, oldProps : Props, newProps : Props) : boolean { - return true; + prepareUpdate(instance : Instance, type : string, oldProps : Props, newProps : Props) : null | {} { + return UPDATE_SIGNAL; }, commitMount(instance : Instance, type : string, newProps : Props) : void { // Noop }, - commitUpdate(instance : Instance, type : string, oldProps : Props, newProps : Props) : void { + commitUpdate(instance : Instance, updatePayload : Object, type : string, oldProps : Props, newProps : Props) : void { instance.prop = newProps.prop; }, diff --git a/src/renderers/shared/fiber/ReactFiberBeginWork.js b/src/renderers/shared/fiber/ReactFiberBeginWork.js index afe933c0503a..dbbb91466956 100644 --- a/src/renderers/shared/fiber/ReactFiberBeginWork.js +++ b/src/renderers/shared/fiber/ReactFiberBeginWork.js @@ -70,8 +70,8 @@ if (__DEV__) { var warnedAboutStatelessRefs = {}; } -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 9b71e2a28b30..c86c263ecd28 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -13,7 +13,6 @@ 'use strict'; import type { Fiber } from 'ReactFiber'; -import type { HostContext } from 'ReactFiberHostContext'; import type { HostConfig } from 'ReactFiberReconciler'; var ReactTypeOfWork = require('ReactTypeOfWork'); @@ -34,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 ) { @@ -51,10 +50,6 @@ module.exports = function( getPublicInstance, } = config; - const { - getRootHostContainer, - } = hostContext; - // Capture errors so they don't interrupt unmounting. function safelyCallComponentWillUnmount(current, instance) { try { @@ -367,9 +362,13 @@ module.exports = function( // Commit the work prepared earlier. const newProps = finishedWork.memoizedProps; const oldProps = current.memoizedProps; - const rootContainerInstance = getRootHostContainer(); const type = finishedWork.type; - commitUpdate(instance, type, oldProps, newProps, rootContainerInstance, finishedWork); + // TODO: Type the updateQueue to be specific to host components. + const updatePayload : null | PL = (finishedWork.updateQueue : any); + finishedWork.updateQueue = null; + if (updatePayload) { + commitUpdate(instance, updatePayload, type, oldProps, newProps, finishedWork); + } } detachRefIfNeeded(current, finishedWork); return; diff --git a/src/renderers/shared/fiber/ReactFiberCompleteWork.js b/src/renderers/shared/fiber/ReactFiberCompleteWork.js index c5277ca04e13..776db30ecc01 100644 --- a/src/renderers/shared/fiber/ReactFiberCompleteWork.js +++ b/src/renderers/shared/fiber/ReactFiberCompleteWork.js @@ -47,8 +47,8 @@ if (__DEV__) { var ReactDebugCurrentFiber = require('ReactDebugCurrentFiber'); } -module.exports = function( - config : HostConfig, +module.exports = function( + config : HostConfig, hostContext : HostContext, ) { const { @@ -186,8 +186,9 @@ module.exports = function( } return null; } - case HostComponent: + case HostComponent: { popHostContext(workInProgress); + const rootContainerInstance = getRootHostContainer(); const type = workInProgress.type; const newProps = workInProgress.memoizedProps; if (current && workInProgress.stateNode != null) { @@ -200,8 +201,12 @@ module.exports = function( // Even better would be if children weren't special cased at all tho. const instance : I = workInProgress.stateNode; const currentHostContext = getHostContext(); - if (prepareUpdate(instance, type, oldProps, newProps, currentHostContext)) { - // This returns true if there was something to update. + const updatePayload = prepareUpdate(instance, type, oldProps, newProps, rootContainerInstance, currentHostContext); + // TODO: Type this specific to this type of component. + workInProgress.updateQueue = (updatePayload : any); + // If the update payload indicates that there is a change or if there + // is a new ref we mark this as an update. + if (updatePayload || current.ref !== workInProgress.ref) { markUpdate(workInProgress); } } else { @@ -214,7 +219,6 @@ module.exports = function( } } - const rootContainerInstance = getRootHostContainer(); const currentHostContext = getHostContext(); // TODO: Move createInstance to beginWork and keep it on a context // "stack" as the parent. Then append children as we go in beginWork @@ -244,7 +248,8 @@ module.exports = function( } } return null; - case HostText: + } + case HostText: { let newText = workInProgress.memoizedProps; if (current && workInProgress.stateNode != null) { const oldText = current.memoizedProps; @@ -268,6 +273,7 @@ module.exports = function( workInProgress.stateNode = textInstance; } return null; + } case CoroutineComponent: return moveCoroutineToHandlerPhase(current, workInProgress); case CoroutineHandlerPhase: diff --git a/src/renderers/shared/fiber/ReactFiberHostContext.js b/src/renderers/shared/fiber/ReactFiberHostContext.js index 11b1437b7e16..1d29b709b6f6 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 52cc90f57d36..dcb226d0a5f6 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, @@ -53,8 +53,8 @@ export type HostConfig = { appendInitialChild(parentInstance : I, child : I | TI) : void, finalizeInitialChildren(parentInstance : I, type : T, props : P, rootContainerInstance : C) : boolean, - prepareUpdate(instance : I, type : T, oldProps : P, newProps : P, hostContext : CX) : boolean, - commitUpdate(instance : I, type : T, oldProps : P, newProps : P, rootContainerInstance : C, internalInstanceHandle : OpaqueNode) : void, + prepareUpdate(instance : I, type : T, oldProps : P, newProps : P, rootContainerInstance : C, hostContext : CX) : null | PL, + commitUpdate(instance : I, updatePayload : PL, type : T, oldProps : P, newProps : P, internalInstanceHandle : OpaqueNode) : void, commitMount(instance : I, type : T, newProps : P, rootContainerInstance : C, internalInstanceHandle : OpaqueNode) : void, shouldSetTextContent(props : P) : boolean, @@ -102,7 +102,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 32f1575d43aa..773ac5c83a26 100644 --- a/src/renderers/shared/fiber/ReactFiberScheduler.js +++ b/src/renderers/shared/fiber/ReactFiberScheduler.js @@ -88,7 +88,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( @@ -104,7 +104,7 @@ module.exports = function(config : HostConfig Date: Tue, 20 Dec 2016 13:06:46 -0800 Subject: [PATCH 02/11] Diff ReactDOMFiber properties in prepareUpdate We now take advantage of the new capability to diff properties early. We do this by generating an update payload in the form of an array with each property name and value that we're about to update. --- src/renderers/dom/fiber/ReactDOMFiber.js | 12 +- .../dom/fiber/ReactDOMFiberComponent.js | 311 ++++++++++++------ 2 files changed, 214 insertions(+), 109 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index cc7a8bb54741..75fb238320df 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -35,6 +35,7 @@ var { createElement, getChildNamespace, setInitialProperties, + diffProperties, updateProperties, } = ReactDOMFiberComponent; var { precacheFiberNode } = ReactDOMComponentTree; @@ -208,7 +209,7 @@ var DOMRenderer = ReactFiberReconciler({ newProps : Props, rootContainerInstance : Container, hostContext : HostContext, - ) : null | Object { + ) : null | Array { if (__DEV__) { const hostContextDev = ((hostContext : any) : HostContextDev); if (typeof newProps.children !== typeof oldProps.children && ( @@ -219,7 +220,7 @@ var DOMRenderer = ReactFiberReconciler({ validateDOMNesting(null, String(newProps.children), null, ownAncestorInfo); } } - return {}; + return diffProperties(domElement, type, oldProps, newProps, rootContainerInstance); }, commitMount( @@ -234,7 +235,7 @@ var DOMRenderer = ReactFiberReconciler({ commitUpdate( domElement : Instance, - updatePayload : Object, + updatePayload : Array, type : string, oldProps : Props, newProps : Props, @@ -242,10 +243,9 @@ var DOMRenderer = ReactFiberReconciler({ ) : void { // Update the internal instance handle so that we know which props are // the current ones. - // TODO: Fix this hack. - var rootContainerInstance = (domElement : any); precacheFiberNode(internalInstanceHandle, domElement); - updateProperties(domElement, type, oldProps, newProps, rootContainerInstance); + // Apply the diff to the DOM node. + updateProperties(domElement, updatePayload, type, oldProps, newProps); }, shouldSetTextContent(props : Props) : boolean { diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index f75c0ad9ebb2..0f5eb655492c 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -301,69 +301,15 @@ function isCustomComponent(tagName, props) { return tagName.indexOf('-') >= 0 || props.is != null; } -/** - * Reconciles the properties by detecting differences in property values and - * updating the DOM as necessary. This function is probably the single most - * critical path for performance optimization. - * - * TODO: Benchmark whether checking for changed values in memory actually - * improves performance (especially statically positioned elements). - * TODO: Benchmark the effects of putting this at the top since 99% of props - * do not change for a given reconciliation. - * TODO: Benchmark areas that can be improved with caching. - */ -function updateDOMProperties( +function setInitialDOMProperties( domElement : Element, rootContainerElement : Element, - lastProps : null | Object, nextProps : Object, - wasCustomComponentTag : boolean, isCustomComponentTag : boolean, ) : void { - var propKey; - var styleName; - var styleUpdates; - for (propKey in lastProps) { - if (nextProps.hasOwnProperty(propKey) || - !lastProps.hasOwnProperty(propKey) || - lastProps[propKey] == null) { - continue; - } - if (propKey === STYLE) { - var lastStyle = lastProps[propKey]; - for (styleName in lastStyle) { - if (lastStyle.hasOwnProperty(styleName)) { - styleUpdates = styleUpdates || {}; - styleUpdates[styleName] = ''; - } - } - } else if (propKey === DANGEROUSLY_SET_INNER_HTML || - propKey === CHILDREN) { - // TODO: Clear innerHTML. This is currently broken in Fiber because we are - // too late to clear everything at this point because new children have - // already been inserted. - } else if (propKey === SUPPRESS_CONTENT_EDITABLE_WARNING) { - // Noop - } else if (registrationNameModules.hasOwnProperty(propKey)) { - // Do nothing for deleted listeners. - } else if (wasCustomComponentTag) { - DOMPropertyOperations.deleteValueForAttribute( - domElement, - propKey - ); - } else if ( - DOMProperty.properties[propKey] || - DOMProperty.isCustomAttribute(propKey)) { - DOMPropertyOperations.deleteValueForProperty(domElement, propKey); - } - } - for (propKey in nextProps) { + for (var propKey in nextProps) { var nextProp = nextProps[propKey]; - var lastProp = - lastProps != null ? lastProps[propKey] : undefined; - if (!nextProps.hasOwnProperty(propKey) || - nextProp === lastProp || - nextProp == null && lastProp == null) { + if (!nextProps.hasOwnProperty(propKey)) { continue; } if (propKey === STYLE) { @@ -374,38 +320,16 @@ function updateDOMProperties( Object.freeze(nextProp); } } - if (lastProp) { - // Unset styles on `lastProp` but not on `nextProp`. - for (styleName in lastProp) { - if (lastProp.hasOwnProperty(styleName) && - (!nextProp || !nextProp.hasOwnProperty(styleName))) { - styleUpdates = styleUpdates || {}; - styleUpdates[styleName] = ''; - } - } - // Update styles that changed since `lastProp`. - for (styleName in nextProp) { - if (nextProp.hasOwnProperty(styleName) && - lastProp[styleName] !== nextProp[styleName]) { - styleUpdates = styleUpdates || {}; - styleUpdates[styleName] = nextProp[styleName]; - } - } - } else { - // Relies on `updateStylesByID` not mutating `styleUpdates`. - styleUpdates = nextProp; - } + // Relies on `updateStylesByID` not mutating `styleUpdates`. + // TODO: call ReactInstrumentation.debugTool.onHostOperation in DEV. + CSSPropertyOperations.setValueForStyles( + domElement, + nextProp, + ); } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { var nextHtml = nextProp ? nextProp[HTML] : undefined; - var lastHtml = lastProp ? lastProp[HTML] : undefined; - // Intentional use of != to avoid catching zero/false. if (nextHtml != null) { - if (lastHtml !== nextHtml) { - setInnerHTML(domElement, '' + nextHtml); - } - } else { - // TODO: It might be too late to clear this if we have children - // inserted already. + setInnerHTML(domElement, nextHtml); } } else if (propKey === CHILDREN) { if (typeof nextProp === 'string') { @@ -433,18 +357,56 @@ function updateDOMProperties( // brings us in line with the same behavior we have on initial render. if (nextProp != null) { DOMPropertyOperations.setValueForProperty(domElement, propKey, nextProp); + } + } + } +} + +function updateDOMProperties( + domElement : Element, + updatePayload : Array, + wasCustomComponentTag : boolean, + isCustomComponentTag : boolean, +) : void { + for (var i = 0; i < updatePayload.length; i+=2) { + var propKey = updatePayload[i]; + var propValue = updatePayload[i + 1]; + if (propKey === STYLE) { + // TODO: call ReactInstrumentation.debugTool.onHostOperation in DEV. + CSSPropertyOperations.setValueForStyles( + domElement, + propValue, + ); + } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { + setInnerHTML(domElement, propValue); + } else if (propKey === CHILDREN) { + setTextContent(domElement, propValue); + } else if (isCustomComponentTag) { + if (propValue != null) { + DOMPropertyOperations.setValueForAttribute( + domElement, + propKey, + propValue + ); + } else { + DOMPropertyOperations.deleteValueForAttribute( + domElement, + propKey + ); + } + } else if ( + DOMProperty.properties[propKey] || + DOMProperty.isCustomAttribute(propKey)) { + // If we're updating to null or undefined, we should remove the property + // from the DOM node instead of inadvertently setting to a string. This + // brings us in line with the same behavior we have on initial render. + if (propValue != null) { + DOMPropertyOperations.setValueForProperty(domElement, propKey, propValue); } else { DOMPropertyOperations.deleteValueForProperty(domElement, propKey); } } } - if (styleUpdates) { - // TODO: call ReactInstrumentation.debugTool.onHostOperation in DEV. - CSSPropertyOperations.setValueForStyles( - domElement, - styleUpdates, - ); - } } // Assumes there is no parent namespace. @@ -592,12 +554,10 @@ var ReactDOMFiberComponent = { assertValidProps(tag, props); - updateDOMProperties( + setInitialDOMProperties( domElement, rootContainerElement, - null, props, - false, isCustomComponentTag ); @@ -626,13 +586,14 @@ var ReactDOMFiberComponent = { } }, - updateProperties( + // Calculate the diff between the two objects. + diffProperties( domElement : Element, tag : string, lastRawProps : Object, nextRawProps : Object, rootContainerElement : Element, - ) : void { + ) : null | Array { if (__DEV__) { validatePropertiesInDevelopment(tag, nextRawProps); } @@ -668,17 +629,161 @@ var ReactDOMFiberComponent = { } assertValidProps(tag, nextProps); - var wasCustomComponentTag = isCustomComponent(tag, lastProps); - var isCustomComponentTag = isCustomComponent(tag, nextProps); + + var propKey; + var styleName; + var styleUpdates = null; + var updatePayload : null | Array = null; + for (propKey in lastProps) { + if (nextProps.hasOwnProperty(propKey) || + !lastProps.hasOwnProperty(propKey) || + lastProps[propKey] == null) { + continue; + } + if (propKey === STYLE) { + var lastStyle = lastProps[propKey]; + for (styleName in lastStyle) { + if (lastStyle.hasOwnProperty(styleName)) { + if (!styleUpdates) { + styleUpdates = {}; + } + styleUpdates[styleName] = ''; + } + } + } else if (propKey === DANGEROUSLY_SET_INNER_HTML || + propKey === CHILDREN) { + // TODO: Clear innerHTML. This is currently broken in Fiber because we are + // too late to clear everything at this point because new children have + // already been inserted. + } else if (propKey === SUPPRESS_CONTENT_EDITABLE_WARNING) { + // Noop + } else if (registrationNameModules.hasOwnProperty(propKey)) { + // This is a special case. If any listener updates we need to ensure + // that the "current" fiber pointer gets updated so we need a commit + // to update this element. + if (!updatePayload) { + updatePayload = []; + } + } else { + // For all other deleted properties we add it to the queue. We use + // the whitelist in the commit phase instead. + (updatePayload = updatePayload || []).push(propKey, null); + } + } + for (propKey in nextProps) { + var nextProp = nextProps[propKey]; + var lastProp = + lastProps != null ? lastProps[propKey] : undefined; + if (!nextProps.hasOwnProperty(propKey) || + nextProp === lastProp || + nextProp == null && lastProp == null) { + continue; + } + if (propKey === STYLE) { + if (__DEV__) { + if (nextProp) { + // Freeze the next style object so that we can assume it won't be + // mutated. We have already warned for this in the past. + Object.freeze(nextProp); + } + } + if (lastProp) { + // Unset styles on `lastProp` but not on `nextProp`. + for (styleName in lastProp) { + if (lastProp.hasOwnProperty(styleName) && + (!nextProp || !nextProp.hasOwnProperty(styleName))) { + if (!styleUpdates) { + styleUpdates = {}; + } + styleUpdates[styleName] = ''; + } + } + // Update styles that changed since `lastProp`. + for (styleName in nextProp) { + if (nextProp.hasOwnProperty(styleName) && + lastProp[styleName] !== nextProp[styleName]) { + if (!styleUpdates) { + styleUpdates = {}; + } + styleUpdates[styleName] = nextProp[styleName]; + } + } + } else { + // Relies on `updateStylesByID` not mutating `styleUpdates`. + if (!styleUpdates) { + if (!updatePayload) { + updatePayload = []; + } + updatePayload.push(propKey, styleUpdates); + } + styleUpdates = nextProp; + } + } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { + var nextHtml = nextProp ? nextProp[HTML] : undefined; + var lastHtml = lastProp ? lastProp[HTML] : undefined; + if (nextHtml) { + if (lastHtml) { + if (lastHtml !== nextHtml) { + (updatePayload = updatePayload || []).push(propKey, '' + nextHtml); + } + } else { + (updatePayload = updatePayload || []).push(propKey, '' + nextHtml); + } + } else { + // TODO: It might be too late to clear this if we have children + // inserted already. + } + } else if (propKey === CHILDREN) { + if (lastProp !== nextProp && ( + typeof nextProp === 'string' || typeof nextProp === 'number' + )) { + (updatePayload = updatePayload || []).push(propKey, '' + nextProp); + } + } else if (propKey === SUPPRESS_CONTENT_EDITABLE_WARNING) { + // Noop + } else if (registrationNameModules.hasOwnProperty(propKey)) { + if (nextProp) { + // We eagerly listen to this even though we haven't committed yet. + ensureListeningTo(rootContainerElement, propKey); + } + if (!updatePayload && lastProp !== nextProp) { + // This is a special case. If any listener updates we need to ensure + // that the "current" fiber pointer gets updated so we need a commit + // to update this element. + updatePayload = []; + } + } else { + // For any other property we always add it to the queue and then we + // filter it out using the whitelist during the commit. + (updatePayload = updatePayload || []).push(propKey, nextProp); + } + } + if (styleUpdates) { + (updatePayload = updatePayload || []).push(STYLE, styleUpdates); + } + return updatePayload; + }, + + // Apply the diff. + updateProperties( + domElement : Element, + updatePayload : Array, + tag : string, + lastRawProps : Object, + nextRawProps : Object + ) : void { + var wasCustomComponentTag = isCustomComponent(tag, lastRawProps); + var isCustomComponentTag = isCustomComponent(tag, nextRawProps); + // Apply the diff. updateDOMProperties( domElement, - rootContainerElement, - lastProps, - nextProps, + updatePayload, wasCustomComponentTag, isCustomComponentTag ); + // TODO: Ensure that an update gets scheduled if any of the special props + // changed. switch (tag) { case 'input': // Update the wrapper around inputs *after* updating props. This has to From b46ff412714a68d944de6cce6e1fff63339505eb Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 19 Dec 2016 18:17:13 -0800 Subject: [PATCH 03/11] Hack around the listener problem --- src/renderers/dom/fiber/ReactDOMFiberComponent.js | 6 +++++- src/renderers/shared/shared/event/EventPluginHub.js | 4 ++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 0f5eb655492c..bbb7c8582c12 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -746,10 +746,14 @@ var ReactDOMFiberComponent = { // We eagerly listen to this even though we haven't committed yet. ensureListeningTo(rootContainerElement, propKey); } - if (!updatePayload && lastProp !== nextProp) { + if (!updatePayload /*&& lastProp !== nextProp*/) { // This is a special case. If any listener updates we need to ensure // that the "current" fiber pointer gets updated so we need a commit // to update this element. + // TODO: It turns out that we always need to update if there are + // listeners. In fact, even this is not enough because not updating + // the "current" pointer will make it so that work in progress + // listeners can fire before they're mounted. updatePayload = []; } } else { diff --git a/src/renderers/shared/shared/event/EventPluginHub.js b/src/renderers/shared/shared/event/EventPluginHub.js index 57d80423340c..6fbeb78c6aef 100644 --- a/src/renderers/shared/shared/event/EventPluginHub.js +++ b/src/renderers/shared/shared/event/EventPluginHub.js @@ -127,6 +127,10 @@ var EventPluginHub = { // live here; needs to be moved to a better place soon if (typeof inst.tag === 'number') { const props = inst.memoizedProps; + if (!props) { + // Work in progress. + return null; + } listener = props[registrationName]; if (shouldPreventMouseEvent(registrationName, inst.type, props)) { return null; From 2017b3a184b10b8163df5307bfc0e887e2d33c89 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 19 Dec 2016 18:50:31 -0800 Subject: [PATCH 04/11] Add todo for handling wasCustomComponentTag --- src/renderers/dom/fiber/ReactDOMFiberComponent.js | 1 + 1 file changed, 1 insertion(+) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index bbb7c8582c12..6640e56b9b60 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -368,6 +368,7 @@ function updateDOMProperties( wasCustomComponentTag : boolean, isCustomComponentTag : boolean, ) : void { + // TODO: Handle wasCustomComponentTag for (var i = 0; i < updatePayload.length; i+=2) { var propKey = updatePayload[i]; var propValue = updatePayload[i + 1]; From c729a9548a970c38c79dc0b151fe15fca087fc26 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Mon, 19 Dec 2016 19:20:30 -0800 Subject: [PATCH 05/11] Always force an update to wrapper components Wrapper components have custom logic that gets applied at the commit phase so we always need to ensure that we schedule an update for them. --- src/renderers/dom/fiber/ReactDOMFiberComponent.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 6640e56b9b60..6e62e87f3f70 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -599,24 +599,30 @@ var ReactDOMFiberComponent = { validatePropertiesInDevelopment(tag, nextRawProps); } + var updatePayload : null | Array = null; + var lastProps : Object; var nextProps : Object; switch (tag) { case 'input': lastProps = ReactDOMFiberInput.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberInput.getHostProps(domElement, nextRawProps); + updatePayload = []; break; case 'option': lastProps = ReactDOMFiberOption.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberOption.getHostProps(domElement, nextRawProps); + updatePayload = []; break; case 'select': lastProps = ReactDOMFiberSelect.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberSelect.getHostProps(domElement, nextRawProps); + updatePayload = []; break; case 'textarea': lastProps = ReactDOMFiberTextarea.getHostProps(domElement, lastRawProps); nextProps = ReactDOMFiberTextarea.getHostProps(domElement, nextRawProps); + updatePayload = []; break; default: lastProps = lastRawProps; @@ -634,7 +640,6 @@ var ReactDOMFiberComponent = { var propKey; var styleName; var styleUpdates = null; - var updatePayload : null | Array = null; for (propKey in lastProps) { if (nextProps.hasOwnProperty(propKey) || !lastProps.hasOwnProperty(propKey) || From 0be90dfd613b8eafe2038df6e5f7d058592154a3 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 11 Jan 2017 13:32:08 -0800 Subject: [PATCH 06/11] Remove rootContainerInstance from commitMount No use case yet and I removed it from commitUpdate earlier. --- src/renderers/dom/fiber/ReactDOMFiber.js | 1 - src/renderers/native/ReactNativeFiber.js | 1 - src/renderers/shared/fiber/ReactFiberCommitWork.js | 4 +--- src/renderers/shared/fiber/ReactFiberReconciler.js | 2 +- 4 files changed, 2 insertions(+), 6 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiber.js b/src/renderers/dom/fiber/ReactDOMFiber.js index 75fb238320df..beeb5bac1346 100644 --- a/src/renderers/dom/fiber/ReactDOMFiber.js +++ b/src/renderers/dom/fiber/ReactDOMFiber.js @@ -227,7 +227,6 @@ var DOMRenderer = ReactFiberReconciler({ domElement : Instance, type : string, newProps : Props, - rootContainerInstance : Container, internalInstanceHandle : Object, ) : void { ((domElement : any) : HTMLButtonElement | HTMLInputElement | HTMLSelectElement | HTMLTextAreaElement).focus(); diff --git a/src/renderers/native/ReactNativeFiber.js b/src/renderers/native/ReactNativeFiber.js index a8a77cfee451..a0ad81281f99 100644 --- a/src/renderers/native/ReactNativeFiber.js +++ b/src/renderers/native/ReactNativeFiber.js @@ -114,7 +114,6 @@ const NativeRenderer = ReactFiberReconciler({ instance : Instance, type : string, newProps : Props, - rootContainerInstance : Object, internalInstanceHandle : Object ) : void { // Noop diff --git a/src/renderers/shared/fiber/ReactFiberCommitWork.js b/src/renderers/shared/fiber/ReactFiberCommitWork.js index c86c263ecd28..37bef63964c2 100644 --- a/src/renderers/shared/fiber/ReactFiberCommitWork.js +++ b/src/renderers/shared/fiber/ReactFiberCommitWork.js @@ -35,7 +35,6 @@ var { module.exports = function( config : HostConfig, - hostContext : HostContext, captureError : (failedFiber : Fiber, error: Error) => ?Fiber ) { @@ -437,8 +436,7 @@ module.exports = function( ) { const type = finishedWork.type; const props = finishedWork.memoizedProps; - const rootContainerInstance = getRootHostContainer(); - commitMount(instance, type, props, rootContainerInstance, finishedWork); + commitMount(instance, type, props, finishedWork); } return; diff --git a/src/renderers/shared/fiber/ReactFiberReconciler.js b/src/renderers/shared/fiber/ReactFiberReconciler.js index dcb226d0a5f6..94c843b4a1a4 100644 --- a/src/renderers/shared/fiber/ReactFiberReconciler.js +++ b/src/renderers/shared/fiber/ReactFiberReconciler.js @@ -55,7 +55,7 @@ export type HostConfig = { prepareUpdate(instance : I, type : T, oldProps : P, newProps : P, rootContainerInstance : C, hostContext : CX) : null | PL, commitUpdate(instance : I, updatePayload : PL, type : T, oldProps : P, newProps : P, internalInstanceHandle : OpaqueNode) : void, - commitMount(instance : I, type : T, newProps : P, rootContainerInstance : C, internalInstanceHandle : OpaqueNode) : void, + commitMount(instance : I, type : T, newProps : P, internalInstanceHandle : OpaqueNode) : void, shouldSetTextContent(props : P) : boolean, resetTextContent(instance : I) : void, From 797cf961352d95bc79eb5bf34906eb6d4ca4d487 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 11 Jan 2017 13:32:33 -0800 Subject: [PATCH 07/11] Use update signal object in test renderer --- src/renderers/testing/ReactTestRendererFiber.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/renderers/testing/ReactTestRendererFiber.js b/src/renderers/testing/ReactTestRendererFiber.js index ab53aeca7217..e9db09a3ce3d 100644 --- a/src/renderers/testing/ReactTestRendererFiber.js +++ b/src/renderers/testing/ReactTestRendererFiber.js @@ -47,6 +47,8 @@ type TextInstance = {| tag : 'TEXT', |}; +const UPDATE_SIGNAL = {}; + var TestRenderer = ReactFiberReconciler({ getRootHostContext() { return emptyObject; @@ -102,17 +104,18 @@ var TestRenderer = ReactFiberReconciler({ type : string, oldProps : Props, newProps : Props, + rootContainerInstance : Container, hostContext : Object, - ) : boolean { - return true; + ) : null | {} { + return UPDATE_SIGNAL; }, commitUpdate( instance : Instance, + updatePayload : {}, type : string, oldProps : Props, newProps : Props, - rootContainerInstance : Container, internalInstanceHandle : Object, ) : void { instance.type = type; @@ -123,7 +126,6 @@ var TestRenderer = ReactFiberReconciler({ instance : Instance, type : string, newProps : Props, - rootContainerInstance : Object, internalInstanceHandle : Object ) : void { // noop From cad6028f6ca0df4dc7a75ef756ab985a4a2898a3 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Wed, 11 Jan 2017 13:43:08 -0800 Subject: [PATCH 08/11] Incorporate 8652 into new algorithm --- src/renderers/dom/fiber/ReactDOMFiberComponent.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 6e62e87f3f70..4cbcc7418a7d 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -727,12 +727,8 @@ var ReactDOMFiberComponent = { } else if (propKey === DANGEROUSLY_SET_INNER_HTML) { var nextHtml = nextProp ? nextProp[HTML] : undefined; var lastHtml = lastProp ? lastProp[HTML] : undefined; - if (nextHtml) { - if (lastHtml) { - if (lastHtml !== nextHtml) { - (updatePayload = updatePayload || []).push(propKey, '' + nextHtml); - } - } else { + if (nextHtml != null) { + if (lastHtml !== nextHtml) { (updatePayload = updatePayload || []).push(propKey, '' + nextHtml); } } else { From 29be1faf8de15f42aa7ff4f68525768b62743925 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 17 Jan 2017 10:42:39 -0800 Subject: [PATCH 09/11] Fix comment --- src/renderers/dom/fiber/ReactDOMFiberComponent.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/renderers/dom/fiber/ReactDOMFiberComponent.js b/src/renderers/dom/fiber/ReactDOMFiberComponent.js index 4cbcc7418a7d..47e0de74cc63 100644 --- a/src/renderers/dom/fiber/ReactDOMFiberComponent.js +++ b/src/renderers/dom/fiber/ReactDOMFiberComponent.js @@ -658,9 +658,7 @@ var ReactDOMFiberComponent = { } } else if (propKey === DANGEROUSLY_SET_INNER_HTML || propKey === CHILDREN) { - // TODO: Clear innerHTML. This is currently broken in Fiber because we are - // too late to clear everything at this point because new children have - // already been inserted. + // Noop. This is handled by the clear text mechanism. } else if (propKey === SUPPRESS_CONTENT_EDITABLE_WARNING) { // Noop } else if (registrationNameModules.hasOwnProperty(propKey)) { From 5ecacf96d8cd66224ad9d4a73d4c5ca2c6bba778 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Tue, 17 Jan 2017 10:54:51 -0800 Subject: [PATCH 10/11] Add failing test for flipping event handlers This illustrates the problem that happens if we store a pointer to the Fiber and then choose not to update that pointer when no properties change. That causes an old Fiber to be retained on the DOM node. Then that Fiber can be reused by the pooling mechanism which then will mutate that Fiber with new event handlers, which makes them active before commit. --- scripts/fiber/tests-failing.txt | 3 + .../dom/fiber/__tests__/ReactDOMFiber-test.js | 83 +++++++++++++++++++ 2 files changed, 86 insertions(+) diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt index 884b8c3dc5eb..ffc7c9d376e5 100644 --- a/scripts/fiber/tests-failing.txt +++ b/scripts/fiber/tests-failing.txt @@ -9,6 +9,9 @@ src/isomorphic/classic/__tests__/ReactContextValidator-test.js src/renderers/dom/__tests__/ReactDOMProduction-test.js * should throw with an error code in production +src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +* should not update event handlers until commit + src/renderers/dom/shared/__tests__/ReactDOMComponent-test.js * should clean up input value tracking * should clean up input textarea tracking diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index ea7f39da297c..41c7f129b791 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js @@ -994,6 +994,89 @@ describe('ReactDOMFiber', () => { ]); }); + it('should not update event handlers until commit', () => { + let ops = []; + const handlerA = () => ops.push('A'); + const handlerB = () => ops.push('B'); + + class Example extends React.Component { + state = { flip: false, count: 0 }; + flip() { + this.setState({ flip: true, count: this.state.count + 1 }); + } + tick() { + this.setState({ count: this.state.count + 1 }); + } + render() { + const useB = !this.props.forceA && this.state.flip; + return ( +
+ ); + } + } + + class Click extends React.Component { + constructor() { + super(); + click(node); + } + render() { + return null; + } + } + + let inst; + ReactDOM.render([ inst = n} />], container); + const node = container.firstChild; + expect(node.tagName).toEqual('DIV'); + + function click(target) { + var fakeNativeEvent = {}; + ReactTestUtils.simulateNativeEventOnNode( + 'topClick', + target, + fakeNativeEvent + ); + } + + click(node); + + expect(ops).toEqual(['A']); + ops = []; + + // Render with the other event handler. + inst.flip(); + + click(node); + + expect(ops).toEqual(['B']); + ops = []; + + // Rerender without changing any props. + inst.tick(); + + click(node); + + expect(ops).toEqual(['B']); + ops = []; + + // Render a flip back to the A handler. The second component invokes the + // click handler during render to simulate a click during an aborted + // render. I use this hack because at current time we don't have a way to + // test aborted ReactDOM renders. + ReactDOM.render([, ], container); + + // Because the new click handler has not yet committed, we should still + // invoke B. + expect(ops).toEqual(['B']); + ops = []; + + // Any click that happens after commit, should invoke A. + click(node); + expect(ops).toEqual(['A']); + + }); + it('should not crash encountering low-priority tree', () => { ReactDOM.render(