feature: Support pure expressions in transform-react-constant-elements #4812

STRML · 2016-11-05T16:26:42Z

Q	A
Bug fix?	yes
Breaking change?	no
New feature?	yes
Deprecations?	no
Spec compliancy?	no
Tests added/pass?	yes
Fixed tickets	None
License	MIT
Doc PR	None

The Problem

Could describe this as a bugfix or feature, but when a pure expression of any sort is in a JSXExpression, the node is marked as mutable and isn't hoisted. This prevents otherwise eligible elements from being found.

For example, the following types of elements should be hoistable:

<div data-text={
  "Some text, " +
  "and some more too."
} />

function render(offset) {
  return function () {
    return <div tabIndex={offset + 1} />;
  };
}

The Fix

The fix is very simple: just add a check for path.isPure() as an out when determining when to hoist.

The expression will remain in the hoisted code, even though it could be statically evaluated. UglifyJS will take care of that in many cases.

Deopt

As noted in facebook/react#3226, it's not safe to reuse elements with mutable props. So the following should not be hoisted:

<div style={{width: 100}} />

This is done via an additional check to path.isObjectExpression(). Please correct me if this is not the way to find a mutable reference. Unfortunately, this will not catch things like frozen objects.

codecov-io · 2016-11-05T16:45:00Z

Codecov Report

Merging #4812 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #4812      +/-   ##
==========================================
+ Coverage   89.41%   89.46%   +0.05%     
==========================================
  Files         204      204              
  Lines        9929     9939      +10     
  Branches     2678     2683       +5     
==========================================
+ Hits         8878     8892      +14     
+ Misses       1051     1047       -4

Impacted Files	Coverage Δ
...gin-transform-react-constant-elements/src/index.js	`100% <100%> (ø)`	✅
packages/babel-traverse/src/scope/index.js	`85.49% <ø> (+0.89%)`	✅

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4edcd02...a741ece. Read the comment docs.

hzoo · 2016-11-05T17:15:10Z

cc @sebmarkbage @gaearon @spicyj

Should note we probably want more deopts given https://github.com/babel/babel/labels/react

STRML · 2016-11-05T18:58:00Z

I've updated this to attempt to evaluate the expression passed:

If the result of the expression is confidently known and is immutable, we use it. This has the following result:

var Foo = React.createClass({
  render: function () {
    return (
      <div data-text={
        "Some text, " +
        "and some more too."
      } />
    );
  }
});
// becomes
var _ref = <div data-text={"Some text, and some more too."} />;

var Foo = React.createClass({
  render: function () {
    return _ref;
  }
});

If the result of the expression is not confidently known, but was deopted by an identifier, we hoist as far as we can, as to support:

function render(offset) {
  return function () {
    return <div tabIndex={offset + 1} />;
  };
}
// becomes
function render(offset) {
  var _ref = <div tabIndex={offset + 1} />;

  return function () {
    return _ref;
  };
}

Going to tackle some of the open react issues separately.

STRML · 2017-01-16T19:28:59Z

Rebased in hopes this will make it in with the rest of the constant-elements fixes @hzoo

gaearon · 2017-01-16T19:30:46Z

Isn't it technically unsafe to hoist any React Element, if props are mutable?

It is safe because we freeze props (in DEV) inside createElement. So it doesn't work in development anyway (and is not supported).

loganfsmyth · 2017-02-13T03:02:21Z

packages/babel-plugin-transform-react-constant-elements/src/index.js

+            // be mutated after render.
+            // https://github.com/facebook/react/issues/3226
+            if (t.isImmutable(resultNode)) {
+              path.replaceWith(resultNode);


Do we want to actually execute this replacement? This visitor at the moment has no side-effects, and it's not obvious that this gets us anything.

Agreed - this is something for Babili or another uglifier. Have pushed a change.

STRML · 2017-02-13T03:19:32Z

Amended to not actually replace the result of a pure expression (this can be done by minifiers). We still need to evaluate it to see if its result is immutable. I'm fairly confident we could safely replace, but it could be a source of bugs and is not really the responsibility of this plugin.

loganfsmyth · 2017-02-13T03:32:19Z

packages/babel-plugin-transform-react-constant-elements/src/index.js

+          const expressionResult = path.evaluate();
+          if (expressionResult.confident) {
+            // We know the result; check its mutability.
+            const resultNode = t.valueToNode(expressionResult.value);


I just realized valueToNode throws if it is given a type that can't be converted to an AST Node, I'm not entirely sure I'm willing to assume that isPure() will cover all those cases. It likely does, but since they are not related from a code standpoint it makes me hesitant.

Thoughts?

Technically the only values we can avoid deopting on are primitives; could skip this call and just check if number, bool, string, null, undefined.

Sounds good to me.

STRML · 2017-02-13T03:43:50Z

Amended.

babel-bot · 2017-02-13T03:49:28Z

Hey @STRML! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time.

Node vnode - linux

* Add new flow preset (#5288) * Fix PathHoister hoisting JSX member expressions on "this". (#5143) The PathHoister ignored member references on "this", causing it to potentially hoist an expression above its function scope. This patch tells the hoister to watch for "this", and if seen, mark the nearest non-arrow function scope as the upper limit for hoistng. This fixes #4397 and is an alternative to #4787. * Fix PathHoister hoisting before bindings. (#5153) Fixes #5149 and enables a few additional safe hoists. * Fix linting error * feature: Support pure expressions in transform-react-constant-elements (#4812) * Fix loose for-of with label (#5298) * Rewrite Hub as interface #5047 (#5050) * Rewrite Hub as interface #5047 * Update index.js * Avoid adding unnecessary closure for block scoping (#5246) When you write ``` for (const x of l) { setTimeout(() => x); } ``` we need to add a closure because the variable is meant to be block-scoped and recreated each time the block runs. We do this. However, we also add the closure when no loop is present. This isn't necessary, because if no loop is present then each piece of code runs at most once. I changed the transform to only add a closure if a variable is referenced from within a loop. * Add greenkeeperio-bot to mention-bot blacklist (#5301) [skip ci] * Upgrade lerna to current beta. (#5300) * Revert "Upgrade lerna to current beta." (#5303) * Add charset so tests work with convert-source-map@>1.4 (#5302) * Add CHANGELOG for 6.23.0 [skip ci] (#5304) * Update babel-types README from script. * v6.23.0 * Revert change that lerna force-committed. * Revert "Rewrite Hub as interface #5047" (#5306) * v6.23.1 * Revert lerna again

#4812)

yiminghe · 2017-10-18T12:11:18Z

packages/babel-plugin-transform-react-constant-elements/src/index.js

+        // It is still safe to hoist that, so long as its result is immutable.
+        // If not, it is not safe to replace as mutable values (like objects) could be mutated after render.
+        // https://github.com/facebook/react/issues/3226
+        if (path.isPure()) {


pure template literals such as

<a className={`ok`}/>

is missed

@yiminghe isnt it covered already by isPure here?

the latest version seems to have template literals check: #5914

while the current stable version babel-traverse@6.26.0 does not @Andarist

STRML force-pushed the jsx-constant-elements-pure-expressions branch from 189820b to 1886a84 Compare November 5, 2016 16:44

STRML changed the title ~~feature: Support pure expressions transform-react-constant-elements~~ feature: Support pure expressions in transform-react-constant-elements Nov 5, 2016

hzoo added i: enhancement PR: New Feature 🚀 A type of pull request used for our changelog categories and removed i: enhancement labels Nov 5, 2016

STRML force-pushed the jsx-constant-elements-pure-expressions branch from 1886a84 to 4a8aa71 Compare November 5, 2016 18:56

hzoo added the area: react label Nov 15, 2016

STRML force-pushed the jsx-constant-elements-pure-expressions branch from 4a8aa71 to 415144a Compare January 16, 2017 19:28

loganfsmyth reviewed Feb 13, 2017

View reviewed changes

STRML force-pushed the jsx-constant-elements-pure-expressions branch from 415144a to 48a7e49 Compare February 13, 2017 03:17

loganfsmyth reviewed Feb 13, 2017

View reviewed changes

STRML force-pushed the jsx-constant-elements-pure-expressions branch from 48a7e49 to 045d23e Compare February 13, 2017 03:43

feature: Support pure expressions in transform-react-constant-elements

a741ece

STRML force-pushed the jsx-constant-elements-pure-expressions branch from 045d23e to a741ece Compare February 13, 2017 03:50

loganfsmyth merged commit 2aa2de8 into babel:master Feb 13, 2017

existentialism pushed a commit that referenced this pull request May 19, 2017

feature: Support pure expressions in transform-react-constant-elements (

4442824

#4812)

yiminghe reviewed Oct 18, 2017

View reviewed changes

lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feature: Support pure expressions in transform-react-constant-elements #4812

feature: Support pure expressions in transform-react-constant-elements #4812

STRML commented Nov 5, 2016 •

edited

Loading

codecov-io commented Nov 5, 2016 •

edited

Loading

hzoo commented Nov 5, 2016 •

edited

Loading

STRML commented Nov 5, 2016 •

edited

Loading

STRML commented Jan 16, 2017

gaearon commented Jan 16, 2017

loganfsmyth Feb 13, 2017

STRML Feb 13, 2017

STRML commented Feb 13, 2017

loganfsmyth Feb 13, 2017

STRML Feb 13, 2017

loganfsmyth Feb 13, 2017

STRML commented Feb 13, 2017

babel-bot commented Feb 13, 2017

yiminghe Oct 18, 2017

Andarist Oct 18, 2017

yiminghe Oct 18, 2017 •

edited

Loading

feature: Support pure expressions in transform-react-constant-elements #4812

feature: Support pure expressions in transform-react-constant-elements #4812

Conversation

STRML commented Nov 5, 2016 • edited Loading

The Problem

The Fix

Deopt

codecov-io commented Nov 5, 2016 • edited Loading

Codecov Report

hzoo commented Nov 5, 2016 • edited Loading

STRML commented Nov 5, 2016 • edited Loading

STRML commented Jan 16, 2017

gaearon commented Jan 16, 2017

loganfsmyth Feb 13, 2017

Choose a reason for hiding this comment

STRML Feb 13, 2017

Choose a reason for hiding this comment

STRML commented Feb 13, 2017

loganfsmyth Feb 13, 2017

Choose a reason for hiding this comment

STRML Feb 13, 2017

Choose a reason for hiding this comment

loganfsmyth Feb 13, 2017

Choose a reason for hiding this comment

STRML commented Feb 13, 2017

babel-bot commented Feb 13, 2017

yiminghe Oct 18, 2017

Choose a reason for hiding this comment

Andarist Oct 18, 2017

Choose a reason for hiding this comment

yiminghe Oct 18, 2017 • edited Loading

Choose a reason for hiding this comment

STRML commented Nov 5, 2016 •

edited

Loading

codecov-io commented Nov 5, 2016 •

edited

Loading

hzoo commented Nov 5, 2016 •

edited

Loading

STRML commented Nov 5, 2016 •

edited

Loading

yiminghe Oct 18, 2017 •

edited

Loading