Skip to content

Commit

Permalink
Diff properties in the commit phase instead of generating an update p…
Browse files Browse the repository at this point in the history
…ayload (#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.
  • Loading branch information
sebmarkbage committed Apr 10, 2023
1 parent dd0619b commit ca41adb
Show file tree
Hide file tree
Showing 19 changed files with 675 additions and 130 deletions.
129 changes: 93 additions & 36 deletions packages/react-dom-bindings/src/client/CSSPropertyOperations.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -64,14 +65,50 @@ 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.
*
* @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, ' +
Expand All @@ -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);
}
}
}
Expand Down Expand Up @@ -167,15 +201,38 @@ function expandShorthandMap(styles) {
* becomes .style.fontVariant = ''
*/
export function validateShorthandPropertyCollisionInDev(
styleUpdates,
prevStyles,
nextStyles,
) {
if (__DEV__) {
if (!nextStyles) {
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) {
Expand All @@ -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,
);
Expand Down
Loading

0 comments on commit ca41adb

Please sign in to comment.