Skip to content

Optimize jsx spreads of object expressions#12557

Merged
nicolo-ribaudo merged 2 commits into
babel:mainfrom
bz2:jsx_flattens_spread
Jan 5, 2021
Merged

Optimize jsx spreads of object expressions#12557
nicolo-ribaudo merged 2 commits into
babel:mainfrom
bz2:jsx_flattens_spread

Conversation

@bz2

@bz2 bz2 commented Dec 23, 2020

Copy link
Copy Markdown
Contributor
Q                       A
Fixed Issues? part of preactjs/preact#2773
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? Kinda?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? No
License MIT

The result of spreading an object expression is known at compile time enabling better code generation without a spread or extends helper.

Replace convertAttribute function with accumulateAttribute to allow multiple attributes to be created from one input.

Rework special case check for spreads when runtime is classic and useSpread is false to check the last added prop rather than the attribute before conversion.

There are edge cases where this will result in vastly smaller code as the helpers which would otherwise be inlined are no longer required. I believe the logic to be sound in all cases, happy to any additional tests that would be helpful.

Generated code differences:

  /*#__PURE__*/
- React.createElement("img", babelHelpers.extends({
-   alt: ""
- }, {
+ React.createElement("img", {
+   alt: "", 
    src,
    title
- }));
+ }); 
  /*#__PURE__*/
  _jsx("img", {
    alt: "", 
-   ...{
-     src,
-     title
-   }   
+   src,
+   title
  }); 
  
  /*#__PURE__*/
- _jsx("blockquote", { ...{
-     cite
-   },  
+ _jsx("blockquote", {
+   cite,
    children: items
  }); 

@babel-bot

babel-bot commented Dec 23, 2020

Copy link
Copy Markdown
Collaborator

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

@codesandbox-ci

codesandbox-ci Bot commented Dec 23, 2020

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit d8afcec:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

The result of spreading an object expression is known at compile time
enabling better code generation without a spread or extends helper.

Replace convertAttribute function with accumulateAttribute to allow
multiple attributes to be created from one input.

Rework special case check for spreads when runtime is classic and
useSpread is false to rewrite generated props after conversion.
@bz2 bz2 force-pushed the jsx_flattens_spread branch from 7cfb335 to 49518aa Compare December 23, 2020 21:04

@nicolo-ribaudo nicolo-ribaudo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks for the PR!

Could you add tests with duplicated props?

<div prop {...{prop}} />;
<div {...{prop}} prop />;

@nicolo-ribaudo nicolo-ribaudo added area: jsx PR: Output optimization 🔬 A type of pull request used for our changelog categories labels Dec 23, 2020
Duplicated keys may be created as per current behaviour, using
`@babel/plugin-transform-duplicate-keys` would allow use when
transpiling to strict.
@bz2

bz2 commented Dec 23, 2020

Copy link
Copy Markdown
Contributor Author

@nicolo-ribaudo Added some tests for duplicated props - no change there from current logic with JSX per #2462 so would also want @babel/plugin-transform-duplicate-keys for transpilation to strict mode.

Also changed the 'classic' logic after first push, to be explicitly two-pass which effectively handles the edge cases (and added tests).

@bz2

bz2 commented Dec 23, 2020

Copy link
Copy Markdown
Contributor Author

Spurious (probably) failure on e2e-babel-old-version:

 FAIL  codemods/babel-plugin-codemod-optional-catch-binding/test/index.js
  ● Test suite failed to run

    ENOMEM: not enough memory, read

@nicolo-ribaudo

Copy link
Copy Markdown
Member

I restarted the e2e tests.
Btw, duplicated properties are supported in strict mode! They only cause problems in some very old browsers, hence the separate plugin.

@bz2

bz2 commented Jan 5, 2021

Copy link
Copy Markdown
Contributor Author

Is there anything I can do to help this get a second review?

@JLHwung JLHwung self-requested a review January 5, 2021 20:37
@nicolo-ribaudo nicolo-ribaudo merged commit ed90f17 into babel:main Jan 5, 2021
@nicolo-ribaudo

Copy link
Copy Markdown
Member

Thanks for the PR!

@bz2

bz2 commented Jan 5, 2021

Copy link
Copy Markdown
Contributor Author

Amazing! Thanks all for the reviews.

@bz2 bz2 deleted the jsx_flattens_spread branch January 8, 2021 09:35
@github-actions github-actions Bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Apr 10, 2021
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Apr 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area: jsx outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Output optimization 🔬 A type of pull request used for our changelog categories

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants