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.
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
fix: skip flattening spread object with __proto__ #14759
fix: skip flattening spread object with __proto__ #14759
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/52541/ |
2b9f640
to
9e3b5c8
Compare
React.createElement("p", { | ||
__proto__: null | ||
}, "text"); | ||
|
||
/*#__PURE__*/ | ||
React.createElement("div", { | ||
"__proto__": null | ||
}, contents); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should spreadElement
be used here?
It seems to be the expected behavior in #14756 (comment).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Yeah the spread element should not be stripped. I will investigate.
@@ -0,0 +1 @@ | |||
<p __proto__={null} class="bar">text</p>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both TS and Babel interpret __proto__
in JSXAttributeKey as the special __proto__
accessor, though JSX spec does not specify such behaviour.
This PR does not change such behaviour, I added a new test case.
e53d3c2
to
f88943e
Compare
// If an object expression is spread element's argument | ||
// it is very likely to contain __proto__ and we should stop | ||
// optimizing spread element | ||
t.isObjectExpression(props[0].argument) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check here is necessary if we have pattern {...{ __proto__: null }}
, but it is not sufficient: {...{...{ foo }}}
, after optimized into {...{ foo }}
by accumulateAttribute
, won't further pass the check here. I think this is fine as practically such pattern is rare.
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
This looks more like a react related PR. |
|
||
<div {...{"__proto__": null}}>{contents}</div>; | ||
|
||
<img alt="" {...{__proto__}} />; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should behave like the computed fields, could you move it to packages/babel-plugin-transform-react-jsx/test/fixtures/react/flattens-spread/input.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. I was gonna update test fixtures when I read your last comment, but then I got a busy weekend.
I marked the PR as I will mark a PR as |
In this PR we avoid optimizing the JSX pattern
{ ... { __proto__: foo }
to{ __proto__: foo }
as JSX spread element evaluates to a plain object created by the globalObject
function.This PR is a follow-up to #12557.