Fix evaluation order with object spread, 2#11471
Conversation
When reviewing babel#11412, I noticed there are more cases where the intermediate values can mutate the spread reference. It's best demonstrated in the [TypeScript](https://github.com/microsoft/TypeScript/blob/eb105efdcd6db8a73f5b983bf329cb7a5eee55e1/src/compiler/transformers/es2018.ts#L272) examples.
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/21253/ |
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
FWIW, I don't "like" it 🙃
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 |
|
I am neutral to introducing those two helpers; if you think that it would improve the output feel free to prepare a PR! |
|
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: Is this duplication expected? I see a lot of empty object literals and duplicated function calls in the output which looks inefficient to me. |
|
I can confirm that this change increases our app's size in React Native by about 60k which is an unfortunate regression. |
|
@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 . |
|
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. |
|
We are using the new experimental JSX transform. |
We had discussed creating a special
In this case, unfortunately not. We need multiple calls in order to get the semantics correctly.
@hzoo: Could we change the output for |
I thought we already did it. If we don't, then we should fix it. |
|
We merged #11520 which changes the output in |
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 },objcan contain a getter prop that mutates thepbinding. If were to continue transforming into_objectSpread({}, obj, { p }), then thepbinding will not be mutated correctly.We may want to just abandon
objectSpreadandobjectSpread2, and introducespreadWithGetandspreadWithGetOwnPropertyhelpers to handle the exact arg as needed.