Merge pull request #4940 from appden/fix-react-constant-elements

Fix React constant element bugs

packages/babel-plugin-transform-es2015-block-scoping/src/index.js

@@ -305,14 +305,14 @@ class BlockScoping {
       this.remap();
     }
 
-    this.updateScopeInfo();
+    this.updateScopeInfo(needsClosure);
 
     if (this.loopLabel && !t.isLabeledStatement(this.loopParent)) {
       return t.labeledStatement(this.loopLabel, this.loop);
     }
   }
 
-  updateScopeInfo() {
+  updateScopeInfo(wrappedInClosure) {
     let scope = this.scope;
     let parentScope = scope.getFunctionParent();
     let letRefs = this.letReferences;
@@ -323,7 +323,12 @@ class BlockScoping {
       if (!binding) continue;
       if (binding.kind === "let" || binding.kind === "const") {
         binding.kind = "var";
-        scope.moveBindingTo(ref.name, parentScope);
+
+        if (wrappedInClosure) {
+          scope.removeBinding(ref.name);
+        } else {
+          scope.moveBindingTo(ref.name, parentScope);
+        }
       }
     }
   }

packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-before-declaration/actual.js

@@ -0,0 +1,11 @@
+function render() {
+  const bar = "bar", renderFoo = () => <foo bar={bar} />;
+
+  return renderFoo();
+}
+
+function render() {
+  const bar = "bar", renderFoo = () => <foo bar={bar} baz={baz} />, baz = "baz";
+
+  return renderFoo();
+}

packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-before-declaration/expected.js

@@ -0,0 +1,15 @@
+function render() {
+  const bar = "bar",
+        _ref = <foo bar={bar} />,
+        renderFoo = () => _ref;
+
+  return renderFoo();
+}
+
+function render() {
+  const bar = "bar",
+        renderFoo = () => <foo bar={bar} baz={baz} />,
+        baz = "baz";
+
+  return renderFoo();
+}

packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-block-scoped-variables/actual.js

@@ -0,0 +1,11 @@
+function render(flag) {
+  if (flag) {
+    let bar = "bar";
+
+    [].map(() => bar);
+
+    return <foo bar={bar} />;
+  }
+
+  return null;
+}

packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-block-scoped-variables/expected.js

@@ -0,0 +1,17 @@
+function render(flag) {
+  if (flag) {
+    var _ret = function () {
+      var bar = "bar";
+
+      [].map(() => bar);
+
+      return {
+        v: <foo bar={bar} />
+      };
+    }();
+
+    if (typeof _ret === "object") return _ret.v;
+  }
+
+  return null;
+}

packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-block-scoped-variables/options.json

@@ -0,0 +1,7 @@
+{
+  "plugins": [
+    "syntax-jsx",
+    "transform-es2015-block-scoping",
+    "transform-react-constant-elements"
+  ]
+}

packages/babel-traverse/src/path/lib/hoister.js

@@ -83,7 +83,7 @@ export default class PathHoister {
         if (binding.kind === "param") continue;
 
         // if this binding appears after our attachment point then don't hoist it
-        if (binding.path.getStatementParent().key > path.key) return;
+        if (this.getAttachmentParentForPath(binding.path).key > path.key) return;
       }
     }
 
@@ -105,16 +105,25 @@ export default class PathHoister {
         return scope.path.get("body").get("body")[0];
       } else {
         // doesn't need to be be attached to this scope
-        return this.getNextScopeStatementParent();
+        return this.getNextScopeAttachmentParent();
       }
     } else if (scope.path.isProgram()) {
-      return this.getNextScopeStatementParent();
+      return this.getNextScopeAttachmentParent();
     }
   }
 
-  getNextScopeStatementParent() {
+  getNextScopeAttachmentParent() {
     let scope = this.scopes.pop();
-    if (scope) return scope.path.getStatementParent();
+    if (scope) return this.getAttachmentParentForPath(scope.path);
+  }
+
+  getAttachmentParentForPath(path) {
+    do {
+      if (!path.parentPath ||
+          (Array.isArray(path.container) && path.isStatement()) ||
+          (path.isVariableDeclarator() && path.parentPath.node.declarations.length > 1))
+        return path;
+    } while ((path = path.parentPath));
   }
 
   hasOwnParamBindings(scope) {
@@ -144,10 +153,10 @@ export default class PathHoister {
 
     // generate declaration and insert it to our point
     let uid = attachTo.scope.generateUidIdentifier("ref");
+    let declarator = t.variableDeclarator(uid, this.path.node);
+
     attachTo.insertBefore([
-      t.variableDeclaration("var", [
-        t.variableDeclarator(uid, this.path.node)
-      ])
+      attachTo.isVariableDeclarator() ? declarator : t.variableDeclaration("var", [declarator])
     ]);
 
     let parent = this.path.parentPath;