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

Modify Babel React JSX Duplicate Children Fix #17101

Merged
merged 2 commits into from
Oct 16, 2019

Conversation

lunaruan
Copy link
Contributor

@lunaruan lunaruan commented Oct 15, 2019

If a JSX element has both a children prop and children (ie. <div children={childOne}>{childTwo}</div>), IE throws an Multiple definitions of a property not allowed in strict mode. This modifies the previous fix (which used an Object.assign) by making the duplicate children a sequence expression on the next prop/child instead so that ordering is preserved. For example:

<Component children={useA()} foo={useB()} children={useC()}>{useD()}</Component>

should compile to

React.jsx(Component, {foo: (useA(), useB()), children: (useC(), useD)})

@sizebot
Copy link

sizebot commented Oct 15, 2019

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 07b0a15

Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ordering and sequencing of calls is extremely important, especially because of hooks. Judging by the summar, this approach is likely to lead to the wrong sequencing order, which means that hook calls (when minified) will be out-of-order:

For example:

<Component children={useA()} foo={useB()} children={useC()}>{useD()}</Component>

Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think the summary of this PR is somewhat different from the implementation. @lunaruan helped by explaining my case and looking into the implementation, this seems to be a non-issue. Sorry for the confusion and great stuff! :)

@lunaruan lunaruan merged commit 3ac0eb0 into facebook:master Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants