From 90cf94f44b157c95176824ea1cac2c0813c2b07f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Mon, 10 Apr 2023 19:09:28 -0400 Subject: [PATCH] Diff properties in the commit phase instead of generating an update payload (#26583) This removes the concept of `prepareUpdate()`, behind a flag. React Native already does everything in the commit phase, but generates a temporary update payload before applying it. React Fabric does it both in the render phase. Now it just moves it to a single host config. For DOM I forked updateProperties into one that does diffing and updating in one pass vs just applying a pre-diffed updatePayload. There are a few downsides of this approach: - If only "children" has changed, we end up scheduling an update to be done in the commit phase. Since we traverse through it anyway, it's probably not much extra. - It does more work in the commit phase so for a large tree that is mostly unchanged, it'll stall longer. - It does some extra work for special cases since that work happens if anything has changed. We no longer have a deep bailout. - The special cases now have to each replicate the "clean up old props" loop, leading to extra code. The benefit is that this doesn't allocate temporary extra objects (possibly multiple per element if the array has to resize). It's less work overall. It also gives us an option to reuse this function for a sync render optimization. Another benefit is that if we do the loop in the commit phase I can do further optimizations by reading all props that I need for special cases in that loop instead of polymorphic reads from props. This is what I'd like to do in future refactors that would be stacked on top of this change. --- .../src/client/CSSPropertyOperations.js | 129 +++-- .../src/client/ReactDOMComponent.js | 482 +++++++++++++++++- .../src/client/ReactFiberConfigDOM.js | 24 +- .../src/ReactFiberConfigFabric.js | 21 +- .../src/createReactNoop.js | 11 - .../src/ReactFiberCommitWork.js | 3 +- .../src/ReactFiberCompleteWork.js | 84 +-- .../src/ReactFiberHydrationContext.js | 14 +- .../ReactIncrementalUpdatesMinimalism-test.js | 10 - .../ReactPersistentUpdatesMinimalism-test.js | 10 - .../src/ReactFiberConfigTestHost.js | 2 +- packages/shared/ReactFeatureFlags.js | 3 + .../forks/ReactFeatureFlags.native-fb.js | 2 + .../forks/ReactFeatureFlags.native-oss.js | 2 + .../forks/ReactFeatureFlags.test-renderer.js | 2 + .../ReactFeatureFlags.test-renderer.native.js | 2 + .../ReactFeatureFlags.test-renderer.www.js | 2 + .../forks/ReactFeatureFlags.www-dynamic.js | 1 + .../shared/forks/ReactFeatureFlags.www.js | 1 + 19 files changed, 675 insertions(+), 130 deletions(-) diff --git a/packages/react-dom-bindings/src/client/CSSPropertyOperations.js b/packages/react-dom-bindings/src/client/CSSPropertyOperations.js index 9c07945aecff9..521f4c67fc66b 100644 --- a/packages/react-dom-bindings/src/client/CSSPropertyOperations.js +++ b/packages/react-dom-bindings/src/client/CSSPropertyOperations.js @@ -11,6 +11,7 @@ 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. @@ -64,6 +65,42 @@ export function createDangerousStringForStyles(styles) { } } +function setValueForStyle(style, styleName, value) { + const isCustomProperty = styleName.indexOf('--') === 0; + if (__DEV__) { + if (!isCustomProperty) { + warnValidStyle(styleName, value); + } + } + + if (value == null || typeof value === 'boolean' || value === '') { + if (isCustomProperty) { + style.setProperty(styleName, ''); + } else if (styleName === 'float') { + style.cssFloat = ''; + } else { + style[styleName] = ''; + } + } else if (isCustomProperty) { + style.setProperty(styleName, value); + } else if ( + typeof value === 'number' && + value !== 0 && + !isUnitlessNumber(styleName) + ) { + style[styleName] = value + 'px'; // Presumes implicit 'px' suffix for unitless numbers + } else { + if (styleName === 'float') { + style.cssFloat = value; + } else { + if (__DEV__) { + checkCSSPropertyStringCoercion(value, styleName); + } + style[styleName] = ('' + value).trim(); + } + } +} + /** * Sets the value for multiple styles on a node. If a value is specified as * '' (empty string), the corresponding style property will be unset. @@ -71,7 +108,7 @@ export function createDangerousStringForStyles(styles) { * @param {DOMElement} node * @param {object} styles */ -export function setValueForStyles(node, styles) { +export function setValueForStyles(node, styles, prevStyles) { if (styles != null && typeof styles !== 'object') { throw new Error( 'The `style` prop expects a mapping from style properties to values, ' + @@ -88,42 +125,39 @@ export function setValueForStyles(node, styles) { } const style = node.style; - for (const styleName in styles) { - if (!styles.hasOwnProperty(styleName)) { - continue; - } - const value = styles[styleName]; - const isCustomProperty = styleName.indexOf('--') === 0; + + if (diffInCommitPhase && prevStyles != null) { if (__DEV__) { - if (!isCustomProperty) { - warnValidStyle(styleName, value); - } + validateShorthandPropertyCollisionInDev(prevStyles, styles); } - if (value == null || typeof value === 'boolean' || value === '') { - if (isCustomProperty) { - style.setProperty(styleName, ''); - } else if (styleName === 'float') { - style.cssFloat = ''; - } else { - style[styleName] = ''; - } - } else if (isCustomProperty) { - style.setProperty(styleName, value); - } else if ( - typeof value === 'number' && - value !== 0 && - !isUnitlessNumber(styleName) - ) { - style[styleName] = value + 'px'; // Presumes implicit 'px' suffix for unitless numbers - } else { - if (styleName === 'float') { - style.cssFloat = value; - } else { - if (__DEV__) { - checkCSSPropertyStringCoercion(value, styleName); + for (const styleName in prevStyles) { + if ( + prevStyles.hasOwnProperty(styleName) && + (styles == null || !styles.hasOwnProperty(styleName)) + ) { + // Clear style + const isCustomProperty = styleName.indexOf('--') === 0; + if (isCustomProperty) { + style.setProperty(styleName, ''); + } else if (styleName === 'float') { + style.cssFloat = ''; + } else { + style[styleName] = ''; } - style[styleName] = ('' + value).trim(); + } + } + for (const styleName in styles) { + const value = styles[styleName]; + if (styles.hasOwnProperty(styleName) && prevStyles[styleName] !== value) { + setValueForStyle(style, styleName, value); + } + } + } else { + for (const styleName in styles) { + if (styles.hasOwnProperty(styleName)) { + const value = styles[styleName]; + setValueForStyle(style, styleName, value); } } } @@ -167,7 +201,7 @@ function expandShorthandMap(styles) { * becomes .style.fontVariant = '' */ export function validateShorthandPropertyCollisionInDev( - styleUpdates, + prevStyles, nextStyles, ) { if (__DEV__) { @@ -175,7 +209,30 @@ export function validateShorthandPropertyCollisionInDev( return; } - const expandedUpdates = expandShorthandMap(styleUpdates); + // Compute the diff as it would happen elsewhere. + const expandedUpdates = {}; + if (prevStyles) { + for (const key in prevStyles) { + if (prevStyles.hasOwnProperty(key) && !nextStyles.hasOwnProperty(key)) { + const longhands = shorthandToLonghand[key] || [key]; + for (let i = 0; i < longhands.length; i++) { + expandedUpdates[longhands[i]] = key; + } + } + } + } + for (const key in nextStyles) { + if ( + nextStyles.hasOwnProperty(key) && + (!prevStyles || prevStyles[key] !== nextStyles[key]) + ) { + const longhands = shorthandToLonghand[key] || [key]; + for (let i = 0; i < longhands.length; i++) { + expandedUpdates[longhands[i]] = key; + } + } + } + const expandedStyles = expandShorthandMap(nextStyles); const warnedAbout = {}; for (const key in expandedUpdates) { @@ -193,7 +250,7 @@ export function validateShorthandPropertyCollisionInDev( "avoid this, don't mix shorthand and non-shorthand properties " + 'for the same value; instead, replace the shorthand with ' + 'separate values.', - isValueEmpty(styleUpdates[originalKey]) ? 'Removing' : 'Updating', + isValueEmpty(nextStyles[originalKey]) ? 'Removing' : 'Updating', originalKey, correctOriginalKey, ); diff --git a/packages/react-dom-bindings/src/client/ReactDOMComponent.js b/packages/react-dom-bindings/src/client/ReactDOMComponent.js index 4e1ecda7857ff..e9398cf076f61 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -69,6 +69,7 @@ import { disableIEWorkarounds, enableTrustedTypesIntegration, enableFilterEmptyStringAttributesDOM, + diffInCommitPhase, } from 'shared/ReactFeatureFlags'; import { mediaEventTypes, @@ -273,6 +274,7 @@ function setProp( key: string, value: mixed, props: any, + prevValue: mixed, ): void { switch (key) { case 'children': { @@ -314,7 +316,7 @@ function setProp( break; } case 'style': { - setValueForStyles(domElement, value); + setValueForStyles(domElement, value, prevValue); break; } // These attributes accept URLs. These must not allow javascript: URLS. @@ -655,6 +657,21 @@ function setProp( ); break; // Properties that should not be allowed on custom elements. + case 'is': { + if (__DEV__) { + if (prevValue != null) { + console.error( + 'Cannot update the "is" prop after it has been initialized.', + ); + } + } + // TODO: We shouldn't actually set this attribute, because we've already + // passed it to createElement. We don't also need the attribute. + // However, our tests currently query for it so it's plausible someone + // else does too so it's break. + setValueForAttribute(domElement, 'is', value); + break; + } case 'innerText': case 'textContent': if (enableCustomElementPropertySupport) { @@ -689,10 +706,11 @@ function setPropOnCustomElement( key: string, value: mixed, props: any, + prevValue: mixed, ): void { switch (key) { case 'style': { - setValueForStyles(domElement, value); + setValueForStyles(domElement, value, prevValue); break; } case 'dangerouslySetInnerHTML': { @@ -859,7 +877,7 @@ export function setInitialProperties( } // defaultChecked and defaultValue are ignored by setProp default: { - setProp(domElement, tag, propKey, propValue, props); + setProp(domElement, tag, propKey, propValue, props, null); } } } @@ -892,7 +910,7 @@ export function setInitialProperties( } // defaultValue are ignored by setProp default: { - setProp(domElement, tag, propKey, propValue, props); + setProp(domElement, tag, propKey, propValue, props, null); } } } @@ -1047,7 +1065,7 @@ export function setInitialProperties( } // defaultChecked and defaultValue are ignored by setProp default: { - setProp(domElement, tag, propKey, propValue, props); + setProp(domElement, tag, propKey, propValue, props, null); } } } @@ -1063,7 +1081,14 @@ export function setInitialProperties( if (propValue == null) { continue; } - setPropOnCustomElement(domElement, tag, propKey, propValue, props); + setPropOnCustomElement( + domElement, + tag, + propKey, + propValue, + props, + null, + ); } return; } @@ -1078,7 +1103,7 @@ export function setInitialProperties( if (propValue == null) { continue; } - setProp(domElement, tag, propKey, propValue, props); + setProp(domElement, tag, propKey, propValue, props, null); } } @@ -1188,15 +1213,428 @@ export function diffProperties( } if (styleUpdates) { if (__DEV__) { - validateShorthandPropertyCollisionInDev(styleUpdates, nextProps.style); + validateShorthandPropertyCollisionInDev(lastProps.style, nextProps.style); } (updatePayload = updatePayload || []).push('style', styleUpdates); } return updatePayload; } -// Apply the diff. export function updateProperties( + domElement: Element, + tag: string, + lastProps: Object, + nextProps: Object, +): void { + if (__DEV__) { + validatePropertiesInDevelopment(tag, nextProps); + } + + 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': { + // Update checked *before* name. + // In the middle of an update, it is possible to have multiple checked. + // When a checked radio tries to change name, browser makes another radio's checked false. + if (nextProps.type === 'radio' && nextProps.name != null) { + updateInputChecked(domElement, nextProps); + } + for (const propKey in lastProps) { + const lastProp = lastProps[propKey]; + if ( + lastProps.hasOwnProperty(propKey) && + lastProp != null && + !nextProps.hasOwnProperty(propKey) + ) { + switch (propKey) { + case 'checked': { + const checked = nextProps.defaultChecked; + const inputElement: HTMLInputElement = (domElement: any); + inputElement.checked = + !!checked && + typeof checked !== 'function' && + checked !== 'symbol'; + break; + } + case 'value': { + // This is handled by updateWrapper below. + break; + } + // defaultChecked and defaultValue are ignored by setProp + default: { + setProp(domElement, tag, propKey, null, nextProps, lastProp); + } + } + } + } + for (const propKey in nextProps) { + const nextProp = nextProps[propKey]; + const lastProp = lastProps[propKey]; + if ( + nextProps.hasOwnProperty(propKey) && + nextProp !== lastProp && + (nextProp != null || lastProp != null) + ) { + switch (propKey) { + case 'checked': { + const checked = + nextProp != null ? nextProp : nextProps.defaultChecked; + const inputElement: HTMLInputElement = (domElement: any); + inputElement.checked = + !!checked && + typeof checked !== 'function' && + checked !== 'symbol'; + break; + } + case 'value': { + // This is handled by updateWrapper below. + break; + } + case 'children': + case 'dangerouslySetInnerHTML': { + if (nextProp != null) { + throw new Error( + `${tag} is a void element tag and must neither have \`children\` nor ` + + 'use `dangerouslySetInnerHTML`.', + ); + } + break; + } + // defaultChecked and defaultValue are ignored by setProp + default: { + setProp(domElement, tag, propKey, nextProp, nextProps, lastProp); + } + } + } + } + + 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, nextProps); + return; + } + case 'select': { + for (const propKey in lastProps) { + const lastProp = lastProps[propKey]; + if ( + lastProps.hasOwnProperty(propKey) && + lastProp != null && + !nextProps.hasOwnProperty(propKey) + ) { + switch (propKey) { + case 'value': { + // This is handled by updateWrapper below. + break; + } + // defaultValue are ignored by setProp + default: { + setProp(domElement, tag, propKey, null, nextProps, lastProp); + } + } + } + } + for (const propKey in nextProps) { + const nextProp = nextProps[propKey]; + const lastProp = lastProps[propKey]; + if ( + nextProps.hasOwnProperty(propKey) && + nextProp !== lastProp && + (nextProp != null || lastProp != null) + ) { + switch (propKey) { + case 'value': { + // This is handled by updateWrapper below. + break; + } + // defaultValue are ignored by setProp + default: { + setProp(domElement, tag, propKey, nextProp, nextProps, lastProp); + } + } + } + } + //