From 7f6201889e8e628eeb53e05d8850ddffa3c2e74a Mon Sep 17 00:00:00 2001 From: Sophie Alpert Date: Fri, 22 Sep 2023 20:24:42 -0700 Subject: [PATCH] Ship diffInCommitPhase (#27409) Performance tests at Meta showed neutral results. --- packages/react-art/src/ReactFiberConfigART.js | 6 - .../src/client/CSSPropertyOperations.js | 8 +- .../src/client/ReactDOMComponent.js | 438 +----------------- .../src/client/ReactFiberConfigDOM.js | 36 +- .../src/ReactFiberConfigFabric.js | 45 +- .../src/ReactFiberConfigNative.js | 15 - .../src/createReactNoop.js | 19 - packages/react-reconciler/README.md | 12 +- .../src/ReactFiberCommitWork.js | 57 +-- .../src/ReactFiberCompleteWork.js | 63 +-- .../src/ReactFiberHydrationContext.js | 16 +- .../src/forks/ReactFiberConfig.custom.js | 1 - .../src/ReactFiberConfigTestHost.js | 13 - packages/shared/ReactFeatureFlags.js | 3 - .../forks/ReactFeatureFlags.native-fb.js | 1 - .../forks/ReactFeatureFlags.native-oss.js | 1 - .../forks/ReactFeatureFlags.test-renderer.js | 1 - .../ReactFeatureFlags.test-renderer.native.js | 1 - .../ReactFeatureFlags.test-renderer.www.js | 1 - .../forks/ReactFeatureFlags.www-dynamic.js | 1 - .../shared/forks/ReactFeatureFlags.www.js | 1 - 21 files changed, 53 insertions(+), 686 deletions(-) diff --git a/packages/react-art/src/ReactFiberConfigART.js b/packages/react-art/src/ReactFiberConfigART.js index 89c5489ebbca6..d0ae53019cac6 100644 --- a/packages/react-art/src/ReactFiberConfigART.js +++ b/packages/react-art/src/ReactFiberConfigART.js @@ -15,10 +15,8 @@ import {DefaultEventPriority} from 'react-reconciler/src/ReactEventPriorities'; const pooledTransform = new Transform(); const NO_CONTEXT = {}; -const UPDATE_SIGNAL = {}; if (__DEV__) { Object.freeze(NO_CONTEXT); - Object.freeze(UPDATE_SIGNAL); } /** Helper Methods */ @@ -312,10 +310,6 @@ export function prepareForCommit() { return null; } -export function prepareUpdate(domElement, type, oldProps, newProps) { - return UPDATE_SIGNAL; -} - export function resetAfterCommit() { // Noop } diff --git a/packages/react-dom-bindings/src/client/CSSPropertyOperations.js b/packages/react-dom-bindings/src/client/CSSPropertyOperations.js index 521f4c67fc66b..55fd43ec479d0 100644 --- a/packages/react-dom-bindings/src/client/CSSPropertyOperations.js +++ b/packages/react-dom-bindings/src/client/CSSPropertyOperations.js @@ -11,7 +11,6 @@ import hyphenateStyleName from '../shared/hyphenateStyleName'; import warnValidStyle from '../shared/warnValidStyle'; import isUnitlessNumber from '../shared/isUnitlessNumber'; import {checkCSSPropertyStringCoercion} from 'shared/CheckStringCoercion'; -import {diffInCommitPhase} from 'shared/ReactFeatureFlags'; /** * Operations for dealing with CSS properties. @@ -126,7 +125,7 @@ export function setValueForStyles(node, styles, prevStyles) { const style = node.style; - if (diffInCommitPhase && prevStyles != null) { + if (prevStyles != null) { if (__DEV__) { validateShorthandPropertyCollisionInDev(prevStyles, styles); } @@ -200,10 +199,7 @@ function expandShorthandMap(styles) { * {font: 'foo', fontVariant: 'bar'} -> {font: 'foo'} * becomes .style.fontVariant = '' */ -export function validateShorthandPropertyCollisionInDev( - prevStyles, - nextStyles, -) { +function validateShorthandPropertyCollisionInDev(prevStyles, nextStyles) { if (__DEV__) { if (!nextStyles) { return; diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index b0f452b29c59e..cbd51de23412d 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -55,7 +55,6 @@ import setTextContent from './setTextContent'; import { createDangerousStringForStyles, setValueForStyles, - validateShorthandPropertyCollisionInDev, } from './CSSPropertyOperations'; import {SVG_NAMESPACE, MATH_NAMESPACE} from './DOMNamespaces'; import isCustomElement from '../shared/isCustomElement'; @@ -74,7 +73,6 @@ import { disableIEWorkarounds, enableTrustedTypesIntegration, enableFilterEmptyStringAttributesDOM, - diffInCommitPhase, } from 'shared/ReactFeatureFlags'; import { mediaEventTypes, @@ -1341,119 +1339,6 @@ export function setInitialProperties( } } -// Calculate the diff between the two objects. -export function diffProperties( - domElement: Element, - tag: string, - lastProps: Object, - nextProps: Object, -): null | Array { - if (__DEV__) { - validatePropertiesInDevelopment(tag, nextProps); - } - - let updatePayload: null | Array = null; - - let propKey; - let styleName; - let styleUpdates = null; - for (propKey in lastProps) { - if ( - nextProps.hasOwnProperty(propKey) || - !lastProps.hasOwnProperty(propKey) || - lastProps[propKey] == null - ) { - continue; - } - switch (propKey) { - case 'style': { - const lastStyle = lastProps[propKey]; - for (styleName in lastStyle) { - if (lastStyle.hasOwnProperty(styleName)) { - if (!styleUpdates) { - styleUpdates = ({}: {[string]: $FlowFixMe}); - } - styleUpdates[styleName] = ''; - } - } - break; - } - default: { - // For all other deleted properties we add it to the queue. We use - // the allowed property list in the commit phase instead. - (updatePayload = updatePayload || []).push(propKey, null); - } - } - } - for (propKey in nextProps) { - const nextProp = nextProps[propKey]; - const lastProp = lastProps != null ? lastProps[propKey] : undefined; - if ( - nextProps.hasOwnProperty(propKey) && - nextProp !== lastProp && - (nextProp != null || lastProp != null) - ) { - switch (propKey) { - case 'style': { - 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 = ({}: {[string]: string}); - } - styleUpdates[styleName] = ''; - } - } - // Update styles that changed since `lastProp`. - for (styleName in nextProp) { - if ( - nextProp.hasOwnProperty(styleName) && - lastProp[styleName] !== nextProp[styleName] - ) { - if (!styleUpdates) { - styleUpdates = ({}: {[string]: $FlowFixMe}); - } - styleUpdates[styleName] = nextProp[styleName]; - } - } - } else { - // Relies on `updateStylesByID` not mutating `styleUpdates`. - if (!styleUpdates) { - if (!updatePayload) { - updatePayload = []; - } - updatePayload.push(propKey, styleUpdates); - } - styleUpdates = nextProp; - } - break; - } - case 'is': - if (__DEV__) { - console.error( - 'Cannot update the "is" prop after it has been initialized.', - ); - } - // Fall through - default: { - (updatePayload = updatePayload || []).push(propKey, nextProp); - } - } - } - } - if (styleUpdates) { - if (__DEV__) { - validateShorthandPropertyCollisionInDev(lastProps.style, nextProps.style); - } - (updatePayload = updatePayload || []).push('style', styleUpdates); - } - return updatePayload; -} - export function updateProperties( domElement: Element, tag: string, @@ -1924,305 +1809,6 @@ export function updateProperties( } } -// Apply the diff. -export function updatePropertiesWithDiff( - domElement: Element, - updatePayload: Array, - tag: string, - lastProps: Object, - nextProps: Object, -): void { - switch (tag) { - case 'div': - case 'span': - case 'svg': - case 'path': - case 'a': - case 'g': - case 'p': - case 'li': { - // Fast track the most common tag types - break; - } - case 'input': { - const name = nextProps.name; - const type = nextProps.type; - const value = nextProps.value; - const defaultValue = nextProps.defaultValue; - const lastDefaultValue = lastProps.defaultValue; - const checked = nextProps.checked; - const defaultChecked = nextProps.defaultChecked; - for (let i = 0; i < updatePayload.length; i += 2) { - const propKey = updatePayload[i]; - const propValue = updatePayload[i + 1]; - switch (propKey) { - case 'type': { - break; - } - case 'name': { - break; - } - case 'checked': { - break; - } - case 'defaultChecked': { - break; - } - case 'value': { - break; - } - case 'defaultValue': { - break; - } - case 'children': - case 'dangerouslySetInnerHTML': { - if (propValue != null) { - throw new Error( - `${tag} is a void element tag and must neither have \`children\` nor ` + - 'use `dangerouslySetInnerHTML`.', - ); - } - break; - } - default: { - setProp( - domElement, - tag, - propKey, - propValue, - nextProps, - lastProps[propKey], - ); - } - } - } - - if (__DEV__) { - const wasControlled = - lastProps.type === 'checkbox' || lastProps.type === 'radio' - ? lastProps.checked != null - : lastProps.value != null; - const isControlled = - nextProps.type === 'checkbox' || nextProps.type === 'radio' - ? nextProps.checked != null - : nextProps.value != null; - - if ( - !wasControlled && - isControlled && - !didWarnUncontrolledToControlled - ) { - console.error( - 'A component is changing an uncontrolled input to be controlled. ' + - 'This is likely caused by the value changing from undefined to ' + - 'a defined value, which should not happen. ' + - 'Decide between using a controlled or uncontrolled input ' + - 'element for the lifetime of the component. More info: https://reactjs.org/link/controlled-components', - ); - didWarnUncontrolledToControlled = true; - } - if ( - wasControlled && - !isControlled && - !didWarnControlledToUncontrolled - ) { - console.error( - 'A component is changing a controlled input to be uncontrolled. ' + - 'This is likely caused by the value changing from a defined to ' + - 'undefined, which should not happen. ' + - 'Decide between using a controlled or uncontrolled input ' + - 'element for the lifetime of the component. More info: https://reactjs.org/link/controlled-components', - ); - didWarnControlledToUncontrolled = true; - } - } - - // Update the wrapper around inputs *after* updating props. This has to - // happen after updating the rest of props. Otherwise HTML5 input validations - // raise warnings and prevent the new value from being assigned. - updateInput( - domElement, - value, - defaultValue, - lastDefaultValue, - checked, - defaultChecked, - type, - name, - ); - return; - } - case 'select': { - const value = nextProps.value; - const defaultValue = nextProps.defaultValue; - const multiple = nextProps.multiple; - const wasMultiple = lastProps.multiple; - for (let i = 0; i < updatePayload.length; i += 2) { - const propKey = updatePayload[i]; - const propValue = updatePayload[i + 1]; - switch (propKey) { - case 'value': { - // This is handled by updateWrapper below. - break; - } - // defaultValue are ignored by setProp - default: { - setProp( - domElement, - tag, - propKey, - propValue, - nextProps, - lastProps[propKey], - ); - } - } - } - //