From 594b4b0fe378c244327ac87ce9209e7ee111d626 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 9 Apr 2023 18:06:59 -0400 Subject: [PATCH 1/4] Add flag --- packages/shared/ReactFeatureFlags.js | 3 +++ packages/shared/forks/ReactFeatureFlags.native-fb.js | 2 ++ packages/shared/forks/ReactFeatureFlags.native-oss.js | 2 ++ packages/shared/forks/ReactFeatureFlags.test-renderer.js | 2 ++ .../shared/forks/ReactFeatureFlags.test-renderer.native.js | 2 ++ packages/shared/forks/ReactFeatureFlags.test-renderer.www.js | 2 ++ packages/shared/forks/ReactFeatureFlags.www-dynamic.js | 1 + packages/shared/forks/ReactFeatureFlags.www.js | 1 + 8 files changed, 15 insertions(+) diff --git a/packages/shared/ReactFeatureFlags.js b/packages/shared/ReactFeatureFlags.js index f3250c17fa33..b301c3b74fe7 100644 --- a/packages/shared/ReactFeatureFlags.js +++ b/packages/shared/ReactFeatureFlags.js @@ -122,6 +122,9 @@ export const enableUseEffectEventHook = __EXPERIMENTAL__; // (handled with an MutationObserver) instead of inline-scripts export const enableFizzExternalRuntime = true; +// Performance related test +export const diffInCommitPhase = __EXPERIMENTAL__; + // ----------------------------------------------------------------------------- // Chopping Block // diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 456d78a191e2..729f1c7b6c27 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -82,5 +82,7 @@ export const enableHostSingletons = true; export const useModernStrictMode = false; export const enableFizzExternalRuntime = false; +export const diffInCommitPhase = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 29d464cb8833..25b14a8bd276 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -72,5 +72,7 @@ export const useModernStrictMode = false; export const enableFizzExternalRuntime = false; export const enableDeferRootSchedulingToMicrotask = true; +export const diffInCommitPhase = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index f102289afedd..aa296a97d7bb 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -72,5 +72,7 @@ export const useModernStrictMode = false; export const enableFizzExternalRuntime = false; export const enableDeferRootSchedulingToMicrotask = true; +export const diffInCommitPhase = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js index e1d66519d50e..2768dd02911d 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native.js @@ -69,5 +69,7 @@ export const enableHostSingletons = true; export const useModernStrictMode = false; export const enableDeferRootSchedulingToMicrotask = true; +export const diffInCommitPhase = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js index 8e7343adb750..42bdea1bb58d 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.www.js @@ -74,5 +74,7 @@ export const useModernStrictMode = false; export const enableFizzExternalRuntime = false; export const enableDeferRootSchedulingToMicrotask = true; +export const diffInCommitPhase = true; + // Flow magic to verify the exports of this file match the original version. ((((null: any): ExportsType): FeatureFlagsType): ExportsType); diff --git a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js index 0926f66fc3a8..a958bf5f6609 100644 --- a/packages/shared/forks/ReactFeatureFlags.www-dynamic.js +++ b/packages/shared/forks/ReactFeatureFlags.www-dynamic.js @@ -25,6 +25,7 @@ export const enableUnifiedSyncLane = __VARIANT__; export const enableTransitionTracing = __VARIANT__; export const enableCustomElementPropertySupport = __VARIANT__; export const enableDeferRootSchedulingToMicrotask = __VARIANT__; +export const diffInCommitPhase = __VARIANT__; // Enable this flag to help with concurrent mode debugging. // It logs information to the console about React scheduling, rendering, and commit phases. diff --git a/packages/shared/forks/ReactFeatureFlags.www.js b/packages/shared/forks/ReactFeatureFlags.www.js index f0157f00f777..98311679f247 100644 --- a/packages/shared/forks/ReactFeatureFlags.www.js +++ b/packages/shared/forks/ReactFeatureFlags.www.js @@ -29,6 +29,7 @@ export const { enableTransitionTracing, enableCustomElementPropertySupport, enableDeferRootSchedulingToMicrotask, + diffInCommitPhase, } = dynamicFeatureFlags; // On WWW, __EXPERIMENTAL__ is used for a new modern build. From c09d9b39460bfdf4902f2c410af4ff922c249331 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Sun, 9 Apr 2023 18:09:48 -0400 Subject: [PATCH 2/4] Diff properties in the commit phase instead of generating an update payload This removes the concept of prepareUpdate(). 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. --- .../src/client/CSSPropertyOperations.js | 49 +- .../src/client/ReactDOMComponent.js | 457 +++++++++++++++++- .../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 +- 11 files changed, 590 insertions(+), 95 deletions(-) diff --git a/packages/react-dom-bindings/src/client/CSSPropertyOperations.js b/packages/react-dom-bindings/src/client/CSSPropertyOperations.js index 9c07945aecff..5ac559147e20 100644 --- a/packages/react-dom-bindings/src/client/CSSPropertyOperations.js +++ b/packages/react-dom-bindings/src/client/CSSPropertyOperations.js @@ -64,6 +64,26 @@ export function createDangerousStringForStyles(styles) { } } +export function clearPreviousStyles(node, prevStyles, nextStyles) { + const style = node.style; + for (const styleName in prevStyles) { + if ( + prevStyles.hasOwnProperty(styleName) && + (nextStyles == null || !nextStyles.hasOwnProperty(styleName)) + ) { + // Clear style + const isCustomProperty = styleName.indexOf('--') === 0; + if (isCustomProperty) { + style.setProperty(styleName, ''); + } else if (styleName === 'float') { + style.cssFloat = ''; + } else { + style[styleName] = ''; + } + } + } +} + /** * Sets the value for multiple styles on a node. If a value is specified as * '' (empty string), the corresponding style property will be unset. @@ -167,7 +187,7 @@ function expandShorthandMap(styles) { * becomes .style.fontVariant = '' */ export function validateShorthandPropertyCollisionInDev( - styleUpdates, + prevStyles, nextStyles, ) { if (__DEV__) { @@ -175,7 +195,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 +236,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 4e1ecda7857f..d6a40cdae326 100644 --- a/packages/react-dom-bindings/src/client/ReactDOMComponent.js +++ b/packages/react-dom-bindings/src/client/ReactDOMComponent.js @@ -50,6 +50,7 @@ import setInnerHTML from './setInnerHTML'; import setTextContent from './setTextContent'; import { createDangerousStringForStyles, + clearPreviousStyles, setValueForStyles, validateShorthandPropertyCollisionInDev, } from './CSSPropertyOperations'; @@ -69,6 +70,7 @@ import { disableIEWorkarounds, enableTrustedTypesIntegration, enableFilterEmptyStringAttributesDOM, + diffInCommitPhase, } from 'shared/ReactFeatureFlags'; import { mediaEventTypes, @@ -273,6 +275,7 @@ function setProp( key: string, value: mixed, props: any, + prevValue: mixed, ): void { switch (key) { case 'children': { @@ -314,6 +317,12 @@ function setProp( break; } case 'style': { + if (diffInCommitPhase && prevValue != null) { + if (__DEV__) { + validateShorthandPropertyCollisionInDev(prevValue, value); + } + clearPreviousStyles(domElement, prevValue, value); + } setValueForStyles(domElement, value); break; } @@ -689,6 +698,7 @@ function setPropOnCustomElement( key: string, value: mixed, props: any, + prevValue: mixed, ): void { switch (key) { case 'style': { @@ -859,7 +869,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 +902,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 +1057,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 +1073,14 @@ export function setInitialProperties( if (propValue == null) { continue; } - setPropOnCustomElement(domElement, tag, propKey, propValue, props); + setPropOnCustomElement( + domElement, + tag, + propKey, + propValue, + props, + null, + ); } return; } @@ -1078,7 +1095,7 @@ export function setInitialProperties( if (propValue == null) { continue; } - setProp(domElement, tag, propKey, propValue, props); + setProp(domElement, tag, propKey, propValue, props, null); } } @@ -1188,15 +1205,415 @@ 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); + } + } + } + } + //