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.

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

Merged
merged 5 commits into from Jul 18, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Jul 14, 2022

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

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 global Object function.

This PR is a follow-up to #12557.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories area: jsx labels Jul 14, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Jul 14, 2022

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

@JLHwung JLHwung force-pushed the do-not-flatten-object-with-__proto__ branch from 2b9f640 to 9e3b5c8 Compare Jul 14, 2022
React.createElement("p", {
__proto__: null
}, "text");

/*#__PURE__*/
React.createElement("div", {
"__proto__": null
}, contents);
Copy link
Contributor

@liuxingbaoyu liuxingbaoyu Jul 15, 2022

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).

Copy link
Contributor Author

@JLHwung JLHwung Jul 15, 2022

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>;
Copy link
Contributor Author

@JLHwung JLHwung Jul 15, 2022

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.

@JLHwung JLHwung force-pushed the do-not-flatten-object-with-__proto__ branch from e53d3c2 to f88943e Compare Jul 15, 2022
// 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)
Copy link
Contributor Author

@JLHwung JLHwung Jul 15, 2022

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.

@JLHwung JLHwung requested a review from nicolo-ribaudo Jul 15, 2022
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
@liuxingbaoyu
Copy link
Contributor

liuxingbaoyu commented Jul 16, 2022

This looks more like a react related PR.
:)


<div {...{"__proto__": null}}>{contents}</div>;

<img alt="" {...{__proto__}} />;
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo Jul 16, 2022

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?

Copy link
Contributor Author

@JLHwung JLHwung Jul 18, 2022

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.

@JLHwung
Copy link
Contributor Author

JLHwung commented Jul 18, 2022

I marked the PR as jsx because it changes how we transpile JSX. JSX is a syntax sugar providing the ability of representing call expressions with XML-like syntax. Though mostly used within React communities, JSX and React are decoupled. And that's why we have React alternatives like preact that also supports JSX.

I will mark a PR as react if it solves a React-related problems, e.g. mark React.createElement as pure so that it can be tree-shaken by bundlers.

@JLHwung JLHwung merged commit ff6304b into babel:main Jul 18, 2022
38 checks passed
@JLHwung JLHwung deleted the do-not-flatten-object-with-__proto__ branch Jul 18, 2022
@babel babel deleted a comment from ZipingL Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: jsx PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants