Use spread syntax for shallow clones #8

remcohaszing · 2023-06-15T13:47:32Z

Initial checklist

I read the support docs
I read the contributing guide
I agree to follow the code of conduct
I searched issues and couldn’t find anything (or linked relevant results below)
If applicable, I’ve added docs and tests

Description of changes

Use object spread syntax for creating shallow clones instead of Object.assign().

Refs mdx-js/mdx#2310

lib/index.js

remcohaszing · 2023-06-15T13:53:52Z

test.js

+        properties: [
+          {type: 'SpreadElement', argument: {type: 'Identifier', name: 'b'}}
+        ]
+      }


Note that this test is about the classic runtime.

Old behaviour: Pass a single JSX spread element by reference
New behavour: Always pass a shallow clone, no exception for a single JSX spread element.

The old behaviour is was consistent with Babel for the classic runtime. The new behaviour is consistent with Babel for the automatic runtime, as well as TypeScript for both the classic and the automatic runtime.

I like the new behaviour, but I’m open to changing it.

Wouldn’t it be nicer not to generate throwaway objects and run faster? :'(

It depends.

I think React creates shallow clones internally anyway, so it’s safe. For other frameworks it may be unsafe to pass objects by reference. However, I haven’t seen any issues with this in practice, which makes me think it’s probably fine to pass by reference.

I also suspect JavaScript engines optimize this anyway, but this is just a guess.

Another difference is that the spread syntax filters own properties.

I think it would be best to keep the new spread syntax, but I’ll gladly change it if you have strong feelings about passing by reference.

think

may

probably

suspect

All the more reason why maybe a benchmark is a good idea! I think as we are generating code, generating fast code for everyone is worth it.

We know that not creating useless objects is fast.
Creating useless objects might be fine in V8. Maybe fine or not in other places. But that’s guesses.

Generating nice code is also nice: definitely one of the goals too. But like, the current code isn’t terrible, but I’m worried that it’s getting a small bit nicer at a perf cost.

That being said I modeled the code after Babel, which presumably has done some perf work, but I didn’t. And I made sure MDX 2 was fast. But might be good to investigate the perf here regardless?

IMO this isn’t about performance. It’s about if we want to make a special case of passing one single JSX spread.

/** @jsx h */ class Person { constructor(firstName, lastName) { this.firstName = firstName this.lastName = lastName } getFullName() { return this.firstName + ' ' + this.lastName } } function h(tagName, props) { console.log(props.getFullName()) } // Logs 'Spongebob Squarepants' <tag {...new Person('Spongebob', 'Squarepants')} /> // Uncaught TypeError: props.getFullName is not a function <tag {...new Person('Spongebob', 'Squarepants')} additional="prop" />

I do think it is important to make sure that the code this project generates performs well?

That being said, your change only affects components that already receive a spread. Most things in MDX are “plain” elements. Very few receive spreads. So in my tests on an MB of MDX which contains markdown and a couple of “HTML” elements, only 2 things changed (_jsx(MDXLayout, and _jsx(_createMdxContent,), which did not affect runtime performance at all.

So performance of this PR is good, this point is resolved.
But your last example raises an interesting point, which might be a breaking change: this code can throw errors where previously it didn’t.

wooorm

Maybe good to benchmark the code? Is that something you’d be interested in checking?

I wonder about speed.

lib/index.js

wooorm · 2023-06-15T14:29:55Z

test.js

+        properties: [
+          {type: 'SpreadElement', argument: {type: 'Identifier', name: 'b'}}
+        ]
+      }


Wouldn’t it be nicer not to generate throwaway objects and run faster? :'(

remcohaszing · 2023-06-17T15:00:54Z

I did some Object.assign vs spread benchmarking. I used the following script:

const object = {a: 'a'}
/** @type {unknown[]} */
const results = []
globalThis.results = results

const runCount = 1e6
console.log(runCount, 'runs')

/*********************************************************************************/

console.log()
console.log('Rest last / new key')
console.time('Spread')
for (let i = 0; i < runCount; i++) {
  results.push({[i]: i, ...object})
}

console.timeEnd('Spread')

console.time('Object.assign')
for (let i = 0; i < runCount; i++) {
  results.push(Object.assign({[i]: i}, object))
}

console.timeEnd('Object.assign')

/*********************************************************************************/

console.log()
console.log('Rest first / new key')
console.time('Spread')
for (let i = 0; i < runCount; i++) {
  results.push({...object, [i]: i})
}

console.timeEnd('Spread')

console.time('Object.assign')
for (let i = 0; i < runCount; i++) {
  results.push(Object.assign({}, object, {[i]: i}))
}

console.timeEnd('Object.assign')

/*********************************************************************************/

console.log()
console.log('Rest last / same key')
console.time('Spread')
for (let i = 0; i < runCount; i++) {
  results.push({b: i, ...object})
}

console.timeEnd('Spread')

console.time('Object.assign')
for (let i = 0; i < runCount; i++) {
  results.push(Object.assign({b: i}, object))
}

console.timeEnd('Object.assign')

/*********************************************************************************/

console.log()
console.log('Rest first / same key')
console.time('Spread')
for (let i = 0; i < runCount; i++) {
  results.push({...object, b: i})
}

console.timeEnd('Spread')

console.time('Object.assign')
for (let i = 0; i < runCount; i++) {
  results.push(Object.assign({}, object, {b: i}))
}

console.timeEnd('Object.assign')

/*********************************************************************************/

const objectB = {b: 'b'}
const objectC = {c: 'c'}

console.log()
console.log('Multiple existing')
console.time('Spread')
for (let i = 0; i < runCount; i++) {
  results.push({...object, ...objectB, ...objectC})
}

console.timeEnd('Spread')

console.time('Object.assign')
for (let i = 0; i < runCount; i++) {
  results.push(Object.assign({}, object, objectB, objectC))
}

console.timeEnd('Object.assign')

/*********************************************************************************/

console.log()
console.log('Multiple mixed')
console.time('Spread')
for (let i = 0; i < runCount; i++) {
  results.push({...object, x: 'x', ...objectB, y: 'y', ...objectC})
}

console.timeEnd('Spread')

console.time('Object.assign')
for (let i = 0; i < runCount; i++) {
  results.push(Object.assign({}, object, {x: 'x'}, objectB, {y: 'y'}, objectC))
}

console.timeEnd('Object.assign')

Node.js v19.9.0 results:

1000000 runs

Rest last / new key
Spread: 482.475ms
Object.assign: 522.934ms

Rest first / new key
Spread: 671.708ms
Object.assign: 1.163s

Rest last / same key
Spread: 84.62ms
Object.assign: 99.156ms

Rest first / same key
Spread: 693.221ms
Object.assign: 530.491ms

Multiple existing
Spread: 1.225s
Object.assign: 228.82ms

Multiple mixed
Spread: 2.458s
Object.assign: 319.635ms

Google Chrome 114 produces similar results.

Firefox 114 results:

1000000 runs 

Rest last / new key 
Spread: 1709ms - timer ended 
Object.assign: 1358ms - timer ended 

Rest first / new key 
Spread: 1441ms - timer ended 
Object.assign: 1351ms - timer ended 

Rest last / same key 
Spread: 440ms - timer ended 
Object.assign: 425ms - timer ended 

Rest first / same key 
Spread: 431ms - timer ended 
Object.assign: 475ms - timer ended 

Multiple existing 
Spread: 737ms - timer ended 
Object.assign: 817ms - timer ended 

Multiple mixed 
Spread: 966ms - timer ended 
Object.assign: 1335ms - timer ended

Epiphany 42 (WebKitGTK 2.38.6) results:

1000000 – "runs"

Rest last / new key
Spread: 439.690ms
Object.assign: 484.594ms

Rest first / new key
Spread: 407.073ms
Object.assign: 1254.225ms

Rest last / same key
Spread: 142.528ms
Object.assign: 210.197ms

Rest first / same key
Spread: 164.701ms
Object.assign: 242.098ms

Multiple existing
Spread: 243.933ms
Object.assign: 256.349ms

Multiple mixed
Spread: 336.746ms
Object.assign: 440.411ms

I was surprised by the situations where Object.assign significantly outperforms spread syntax in Node.js. I modified the last benchmark case and reran with Node.js:

console.log()
console.log('Multiple mixed')
console.time('Spread')
for (let i = 0; i < runCount; i++) {
  results.push({x: 'x', ...objectB, y: 'y', ...objectC})
}

console.timeEnd('Spread')

console.time('Object.assign')
for (let i = 0; i < runCount; i++) {
  results.push(Object.assign({x: 'x'}, objectB, {y: 'y'}, objectC))
}

console.timeEnd('Object.assign')

This produces:

Multiple mixed
Spread: 123.48ms
Object.assign: 168.071ms

I think this shows both spread syntax and Object.assign have situations where they perform better. Especially in v8 results are very situational.

wooorm · 2023-06-20T13:38:43Z

I compared the differences around spreads that are generated by this project currently, the PR, Babel, and TS:

Input	Current (classic)	Current (auto)	Proposed (classic)	Proposed (auto)	Babel (classic)	Babel (auto)	TS (classic)	TS (auto)
`<a {...b} />`	`h('a', b)`	`_jsx('a', b)`	`h('a', {...b})`	`_jsx('a', {...b})`	`h('a', b)`	`_jsx('a', {...b})`	`h('a', {...b})`	`_jsx('a', {...b})`
`<a key='b' {...c} />`	`h('a', Object.assign({key: 'b'}, c))`	`_jsx('a', c, 'b')`	`h('a', {key: 'b', ...c})`	`_jsx('a', {...c}, 'b')`	`h('a', Object.assign({key: 'b'}, c))`	`_jsx('a', {...c}, 'b')`	`h('a', {key: 'b', ...c})`	`_jsx('a', {...c}, 'b')`
`<a {...b} key='c' />`	`h('a', Object.assign({}, b, {key: 'c'}))`	💥	`h('a', {...b, key: 'c'})`	💥	`h('a', Object.assign({}, b, {key: 'c'}))`	`h('a', {...b, key: 'c'})`	`h('a', {...b, key: 'c'})`	`h('a', {...b, key: 'c'})`
`<a b='c' {...d} />`	`h('a', Object.assign({b: 'c'}, d))`	`_jsx('a', Object.assign({b: 'c'}, d))`	`h('a', {b: 'c', ...d})`	`_jsx('a', {b: 'c', ...d})`	`h('a', Object.assign({b: 'c'}, d))`	`_jsx('a', {b: 'c', ...d})`	`h('a', {b: 'c', ...d})`	`_jsx('a', {b: 'c', ...d})`
`<a {...b} c='d' />`	`h('a', Object.assign({}, b, {c: 'd'}))`	`_jsx('a', Object.assign({}, b, {c: 'd'}))`	`h('a', {...b, c: 'd'})`	`_jsx('a', {...b, c: 'd'})`	`h('a', Object.assign({}, b, {c: 'd'}))`	`_jsx('a', {...b, c: 'd'})`	`h('a', {...b, c: 'd'})`	`_jsx('a', {...b, c: 'd'})`

Your proposal matches TS and it matches Babel (auto).
The project currently matches Babel (classic).

There are two things in your proposal:
a) Object.assign -> {...}
b) Always spreading

I think a) is fine.
There could theoretically be performance differences, but we found that both randomly performed better or worse in slightly different cases and in different browser, so it doesn’t seem to be better or worse.
There could theoretically be support differences. I don’t see anything real on MDN, it’s all green after 2016.

b) is probably fine.
As you’ve shown before, turning <a {...b} /> into _jsx('a', b) (current) into _jsx('a', {...b}) (proposed) is different.
It’s a bit more bytes. For what?
We align with Babel (classic), why change that?
It’s breaking: assuming only one thing was passed, currently something with non-enumerable fields can be passed, whereas now those will be ignored. This could be considered a bug fix tho, and presumably it isn’t depended on much.

remcohaszing · 2023-06-20T14:47:33Z

Interestingly, Babel’s behavour depends on the useSpread option.

If this is disabled (default), it passes by reference. If enabled, it creates a shallow copy.

Given the following code:

<div {...props} />

Using the following babel.config.js:

module.exports = {
  plugins: [
    [
      '@babel/plugin-transform-react-jsx',
      {useSpread: true}
    ]
  ]
}

Yields:

/*#__PURE__*/React.createElement("div", {
  ...props
});

Using the following babel.config.js:

module.exports = {
  plugins: [
    [
      '@babel/plugin-transform-react-jsx',
      // Default
      {useSpread: false}
    ]
  ]
}

Yields:

/*#__PURE__*/React.createElement("div", props);

SWC always passes by reference for the classic runtime, and a shallow copy for the automatic runtime.

ESBuild always creates a shallow copy, just like TypeScript

I think this shows generally that most tools / situations result in a shallow copy. I also found some discussion on Babel’s implementation in babel/babel#11419.

wooorm · 2023-06-20T15:17:00Z

SWC always passes by reference for the classic runtime, and a shallow copy for the automatic runtime.

Likely, both SWC and I were inspired by Babel, but did not care about the legacy of “useSpread” or other weird options.

Interesting thread! It makes sense that frameworks want a mutable copy that they own, shallowly. But as it isn’t like that yet, no framework can assume that, so it’s rather useless now.

How do you assess the risk of potentially breaking someone? I agree it’s a tiny chance.

wooorm · 2023-06-21T17:11:27Z

I guess I can just do a breaking change as I’m doing majors anyway!

remcohaszing · 2023-06-22T08:15:58Z

I don’t expect this to break anyone’s code, but there is a small chance. I think I’d make it a major release just to be sure. Also I’d drop Node.js 14 support while at it.

This affects the assignment of `_components`. For JSX see syntax-tree/estree-util-build-jsx#8

wooorm · 2023-07-19T12:22:32Z

released, thanks! :)

Use spread syntax for shallow clones

c579bf0

remcohaszing added 🦋 type/enhancement This is great to have 🤞 phase/open Post is being triaged manually labels Jun 15, 2023

remcohaszing requested review from wooorm and ChristianMurphy June 15, 2023 13:47

remcohaszing commented Jun 15, 2023

View reviewed changes

wooorm reviewed Jun 15, 2023

View reviewed changes

ChristianMurphy approved these changes Jun 15, 2023

View reviewed changes

remcohaszing added 3 commits June 16, 2023 14:15

Rename properties to fields

9fbb129

Fit statement on single line

11912e4

Replace destructuring syntax

2511415

remcohaszing added a commit to mdx-js/mdx that referenced this pull request Jul 17, 2023

Change Object.assign() calls to object spread

1291c30

This affects the assignment of `_components`. For JSX see syntax-tree/estree-util-build-jsx#8

remcohaszing mentioned this pull request Jul 17, 2023

Object spread mdx-js/mdx#2328

Merged

remcohaszing added a commit to mdx-js/mdx that referenced this pull request Jul 17, 2023

Change Object.assign() calls to object spread

98debd0

This affects the assignment of `_components`. For JSX see syntax-tree/estree-util-build-jsx#8

wooorm merged commit 6e422a2 into main Jul 19, 2023

wooorm deleted the object-spread branch July 19, 2023 08:42

This comment has been minimized.

Sign in to view

wooorm added the 💪 phase/solved Post is done label Jul 19, 2023

github-actions bot removed the 🤞 phase/open Post is being triaged manually label Jul 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use spread syntax for shallow clones #8

Use spread syntax for shallow clones #8

remcohaszing commented Jun 15, 2023

remcohaszing Jun 15, 2023

wooorm Jun 15, 2023

remcohaszing Jun 16, 2023

wooorm Jun 16, 2023

remcohaszing Jun 17, 2023 •

edited

Loading

wooorm Jun 20, 2023

wooorm left a comment

wooorm Jun 15, 2023

remcohaszing commented Jun 17, 2023

wooorm commented Jun 20, 2023

remcohaszing commented Jun 20, 2023

wooorm commented Jun 20, 2023

wooorm commented Jun 21, 2023

remcohaszing commented Jun 22, 2023

This comment has been minimized.

wooorm commented Jul 19, 2023

Use spread syntax for shallow clones #8

Use spread syntax for shallow clones #8

Conversation

remcohaszing commented Jun 15, 2023

Initial checklist

Description of changes

remcohaszing Jun 15, 2023

Choose a reason for hiding this comment

wooorm Jun 15, 2023

Choose a reason for hiding this comment

remcohaszing Jun 16, 2023

Choose a reason for hiding this comment

wooorm Jun 16, 2023

Choose a reason for hiding this comment

remcohaszing Jun 17, 2023 • edited Loading

Choose a reason for hiding this comment

wooorm Jun 20, 2023

Choose a reason for hiding this comment

wooorm left a comment

Choose a reason for hiding this comment

wooorm Jun 15, 2023

Choose a reason for hiding this comment

remcohaszing commented Jun 17, 2023

wooorm commented Jun 20, 2023

remcohaszing commented Jun 20, 2023

wooorm commented Jun 20, 2023

wooorm commented Jun 21, 2023

remcohaszing commented Jun 22, 2023

This comment has been minimized.

wooorm commented Jul 19, 2023

remcohaszing Jun 17, 2023 •

edited

Loading