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(transform-react-constant-elements): wrap hoisted values to enable tree-shaking #11102

Closed
wants to merge 2 commits into from

Conversation

yannickcr
Copy link

Q                       A
Fixed Issues? Fixes #11037
Patch: Bug Fix? Yes
Major: Breaking Change? I don't think, plugin output is different but it should not be a breaking change
Minor: New Feature? No
Tests Added + Pass? Updated output snapshots
Any Dependency Changes? No
License MIT

As described in #11037, the current code generated by @babel/plugin-transform-react-constant-elements is not tree-shakable since the #__PURE__ annotation only works on function calls (terser/terser#513).

The proposed solution (terser/terser#513 (comment)) is to wrap the hoisted values in an IIFE to make them tree-shakable. This is what this change intent to do.

No new tests were added but the existing snapshots were updated to match the new output.

@nicolo-ribaudo nicolo-ribaudo added area: jsx PR: Performance 🏃‍♀️ A type of pull request used for our changelog categories labels Feb 6, 2020
@nicolo-ribaudo
Copy link
Member

The problem that this PR solver is not that terser is unable to remove compiled JSX elements, it's that we don't add the __PURE__ annotation to nested elements.

Try pasting this code in Terser's REPL:

(() => {

let _ref =
/*#__PURE__*/
React.createElement("svg1", null, React.createElement("path1", null));

let _ref2 =
/*#__PURE__*/
(() => React.createElement("svg2", null, React.createElement("path2", null)))();

})();

You'll notice that both svg1 and svg2 are removed, but only path2 and not path1.

nicolo-ribaudo
nicolo-ribaudo previously approved these changes Feb 6, 2020
@nicolo-ribaudo nicolo-ribaudo dismissed their stale review February 6, 2020 17:23

The output of some tests (babel-plugin-transform-react-jsx/react › optimisation.react) seems wrong 🤔

(() => <div>
{_ref3}
{_ref4}
</div>)();
Copy link
Member

Choose a reason for hiding this comment

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

This should be kept as a single element, not as 3 separate elements.
The arrow function is not needed if the elements don't have impure children anyway.

I think that a possible solution would be adding the __PURE__ annotation to every child, and avoid the IIFE, but let's wait for other opinions. Or maybe we could ask the terser team to support marking a whole expression as pure, using __PURE__ (expr) 😛

@JLHwung
Copy link
Contributor

JLHwung commented Feb 6, 2020

Oops, I am also working on this issue and #11105 is just submitted.

@yannickcr
Copy link
Author

yannickcr commented Feb 8, 2020

@JLHwung Oh! Seems that unlike me you're managed to skip hoisting for children elements.

Let's continue on #11105 then, I'll close this one 🙂

@yannickcr yannickcr closed this Feb 8, 2020
@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 May 10, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2020
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: Performance 🏃‍♀️ A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@babel/plugin-transform-react-constant-elements generates non-tree-shakeable output
3 participants