Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fast JSX: Don't clone props object #28768

Merged
merged 1 commit into from
Apr 5, 2024
Merged

Fast JSX: Don't clone props object #28768

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions packages/react/src/__tests__/ReactJSXRuntime-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,4 +374,37 @@ describe('ReactJSXRuntime', () => {
}
expect(didCall).toBe(false);
});

// @gate enableRefAsProp
// @gate disableStringRefs
it('does not clone props object if key is not spread', async () => {
const config = {
foo: 'foo',
bar: 'bar',
};

const element = __DEV__
? JSXDEVRuntime.jsxDEV('div', config)
: JSXRuntime.jsx('div', config);
expect(element.props).toBe(config);

const configWithKey = {
foo: 'foo',
bar: 'bar',
// This only happens when the key is spread onto the element. A statically
// defined key is passed as a separate argument to the jsx() runtime.
key: 'key',
};

let elementWithSpreadKey;
expect(() => {
elementWithSpreadKey = __DEV__
? JSXDEVRuntime.jsxDEV('div', configWithKey)
: JSXRuntime.jsx('div', configWithKey);
}).toErrorDev(
'A props object containing a "key" prop is being spread into JSX',
{withoutStack: true},
);
expect(elementWithSpreadKey.props).not.toBe(configWithKey);
});
});
112 changes: 68 additions & 44 deletions packages/react/src/jsx/ReactJSXElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,11 +314,6 @@ function ReactElement(type, key, _ref, self, source, owner, props) {
* @param {string} key
*/
export function jsxProd(type, config, maybeKey) {
let propName;

// Reserved names are extracted
const props = {};

let key = null;
let ref = null;

Expand Down Expand Up @@ -351,22 +346,39 @@ export function jsxProd(type, config, maybeKey) {
}
}

// Remaining properties are added to a new props object
for (propName in config) {
if (
hasOwnProperty.call(config, propName) &&
// Skip over reserved prop names
propName !== 'key' &&
(enableRefAsProp || propName !== 'ref')
) {
if (enableRefAsProp && !disableStringRefs && propName === 'ref') {
props.ref = coerceStringRef(
config[propName],
ReactCurrentOwner.current,
type,
);
} else {
props[propName] = config[propName];
let props;
if (enableRefAsProp && disableStringRefs && !('key' in config)) {
// If key was not spread in, we can reuse the original props object. This
// only works for `jsx`, not `createElement`, because `jsx` is a compiler
// target and the compiler always passes a new object. For `createElement`,
// we can't assume a new object is passed every time because it can be
// called manually.
//
// Spreading key is a warning in dev. In a future release, we will not
// remove a spread key from the props object. (But we'll still warn.) We'll
// always pass the object straight through.
props = config;
} else {
// We need to remove reserved props (key, prop, ref). Create a fresh props
// object and copy over all the non-reserved props. We don't use `delete`
// because in V8 it will deopt the object to dictionary mode.
props = {};
for (const propName in config) {
if (
hasOwnProperty.call(config, propName) &&
// Skip over reserved prop names
propName !== 'key' &&
(enableRefAsProp || propName !== 'ref')
) {
if (enableRefAsProp && !disableStringRefs && propName === 'ref') {
props.ref = coerceStringRef(
config[propName],
ReactCurrentOwner.current,
type,
);
} else {
props[propName] = config[propName];
}
}
}
}
Expand All @@ -375,7 +387,7 @@ export function jsxProd(type, config, maybeKey) {
// Resolve default props
if (type && type.defaultProps) {
const defaultProps = type.defaultProps;
for (propName in defaultProps) {
for (const propName in defaultProps) {
if (props[propName] === undefined) {
props[propName] = defaultProps[propName];
}
Expand Down Expand Up @@ -538,11 +550,6 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) {
}
}

let propName;

// Reserved names are extracted
const props = {};

let key = null;
let ref = null;

Expand Down Expand Up @@ -578,22 +585,39 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) {
}
}

// Remaining properties are added to a new props object
for (propName in config) {
if (
hasOwnProperty.call(config, propName) &&
// Skip over reserved prop names
propName !== 'key' &&
(enableRefAsProp || propName !== 'ref')
) {
if (enableRefAsProp && !disableStringRefs && propName === 'ref') {
props.ref = coerceStringRef(
config[propName],
ReactCurrentOwner.current,
type,
);
} else {
props[propName] = config[propName];
let props;
if (enableRefAsProp && disableStringRefs && !('key' in config)) {
// If key was not spread in, we can reuse the original props object. This
// only works for `jsx`, not `createElement`, because `jsx` is a compiler
// target and the compiler always passes a new object. For `createElement`,
// we can't assume a new object is passed every time because it can be
// called manually.
//
// Spreading key is a warning in dev. In a future release, we will not
// remove a spread key from the props object. (But we'll still warn.) We'll
// always pass the object straight through.
props = config;
} else {
// We need to remove reserved props (key, prop, ref). Create a fresh props
// object and copy over all the non-reserved props. We don't use `delete`
// because in V8 it will deopt the object to dictionary mode.
props = {};
for (const propName in config) {
if (
hasOwnProperty.call(config, propName) &&
// Skip over reserved prop names
propName !== 'key' &&
(enableRefAsProp || propName !== 'ref')
) {
if (enableRefAsProp && !disableStringRefs && propName === 'ref') {
props.ref = coerceStringRef(
config[propName],
ReactCurrentOwner.current,
type,
);
} else {
props[propName] = config[propName];
}
}
}
}
Expand All @@ -602,7 +626,7 @@ export function jsxDEV(type, config, maybeKey, isStaticChildren, source, self) {
// Resolve default props
if (type && type.defaultProps) {
const defaultProps = type.defaultProps;
for (propName in defaultProps) {
for (const propName in defaultProps) {
if (props[propName] === undefined) {
props[propName] = defaultProps[propName];
}
Expand Down