Fix evaluation order with object spread, 2

[jridgewell]

Q	A
Fixed Issues?
Patch: Bug Fix?	Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

When reviewing #11412, I noticed in the TypeScript link that there are more cases where the intermediate values can mutate the spread reference. In { ...obj, p }, obj can contain a getter prop that mutates the p binding. If were to continue transforming into _objectSpread({}, obj, { p }), then the p binding will not be mutated correctly.

We may want to just abandon objectSpread and objectSpread2, and introduce spreadWithGet and spreadWithGetOwnProperty helpers to handle the exact arg as needed.

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/21253/

[nicolo-ribaudo]

FWIW, I don't "like" it 🙃

[nicolo-ribaudo]

introduce spreadWithGet and spreadWithGetOwnProperty helpers to handle the exact arg as needed.

Would these functions only take two arguments rather than any number of them?

[jridgewell]

Would these functions only take two arguments rather than any number of them?

Yah, essentially. But it would eliminate the weird empty objects we get in var simpleOutput = _objectSpread(_objectSpread({}, pureA), {}, { test: '1' }, pureB);

[nicolo-ribaudo]

I am neutral to introducing those two helpers; if you think that it would improve the output feel free to prepare a PR!

[cpojer]

When upgrading a large codebase from 7.9.4 to 7.9.6 I notice that a lot of code now receives extra spread calls: babelHelpers.objectSpread2(babelHelpers.objectSpread2({}, sharedData), {}, {…}).

Is this duplication expected? I see a lot of empty object literals and duplicated function calls in the output which looks inefficient to me.

[cpojer]

I can confirm that this change increases our app's size in React Native by about 60k which is an unfortunate regression.

[hzoo]

@cpojer this is good to know, we may need to see the cases where it's increasing the size for what usage.. Yeah we need to look into how to add this in without affecting it when it doesn't need to be there if possible.

Makes me think in general we also need a larger discussion about how we handle spec compliance and default behavior. Tradeoffs of this since fixing bugs is always good but has a cost. And introducing new flags to support what should be correct behavior isn't great either? Probably needs judgement on per case basis at least in the past but we need automated check on effect of codesize @existentialism .

[cpojer]

Could we not move more stuff into the helper? I don't know why empty objects need to be created so much and then passed around.

[developit]

@cpojer any chance you're using the useSpread option? If not producing modern output, that will trigger this helper instead of the JSX-specific assign() polyfill (which doesn't have to account for these edge cases).

[cpojer]

We are using the new experimental JSX transform.

[jridgewell]

I see a lot of empty object literals and duplicated function calls in the output which looks inefficient to me.

We had discussed creating a special spreadWithGet and spreadWithGetOwnProperty helpers, instead of reusing objectSpread helper for both cases. This would get rid of the weird empty objects (which are caused by the weird API we did for objectSpread).

Could we not move more stuff into the helper?

In this case, unfortunately not. We need multiple calls in order to get the semantics correctly.

Makes me think in general we also need a larger discussion about how we handle spec compliance and default behavior. Tradeoffs of this since fixing bugs is always good but has a cost. And introducing new flags to support what should be correct behavior isn't great either?

@hzoo: Could we change the output for loose mode?

[nicolo-ribaudo]

Could we change the output for loose mode?

I thought we already did it. If we don't, then we should fix it.

[hzoo]

We merged #11520 which changes the output in loose