From 3b4b3656a84856150631cb3533379999e29137b3 Mon Sep 17 00:00:00 2001 From: Sebastian McKenzie Date: Wed, 27 Jul 2016 15:54:21 +0100 Subject: [PATCH] Fix React constant elements transform from hoisting elements to positions where their referenced bindings haven't been evaluated yet (#3596) --- .../actual.js | 14 ++++++++ .../expected.js | 16 +++++++++ .../actual.js | 16 +++++++++ .../expected.js | 18 ++++++++++ .../actual.js | 16 +++++++++ .../expected.js | 20 +++++++++++ .../actual.js | 14 ++++++++ .../expected.js | 16 +++++++++ .../babel-traverse/src/path/lib/hoister.js | 33 +++++++++++++++++-- 9 files changed, 161 insertions(+), 2 deletions(-) create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-2/actual.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-2/expected.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-3/actual.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-3/expected.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-4/actual.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-4/expected.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope/actual.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope/expected.js diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-2/actual.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-2/actual.js new file mode 100644 index 0000000000..bd66737f0c --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-2/actual.js @@ -0,0 +1,14 @@ +const AppItem = () => { + return
child
; +}; + +export default class App extends React.Component { + render() { + return ( +
+

Parent

+ +
+ ); + } +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-2/expected.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-2/expected.js new file mode 100644 index 0000000000..eb5e2b2e0c --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-2/expected.js @@ -0,0 +1,16 @@ +var _ref =
child
; + +const AppItem = () => { + return _ref; +}; + +var _ref2 =
+

Parent

+ +
; + +export default class App extends React.Component { + render() { + return _ref2; + } +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-3/actual.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-3/actual.js new file mode 100644 index 0000000000..b90a17e116 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-3/actual.js @@ -0,0 +1,16 @@ +(function () { + class App extends React.Component { + render() { + return ( +
+

Parent

+ +
+ ); + } + } + + const AppItem = () => { + return
child
; + }; +}); diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-3/expected.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-3/expected.js new file mode 100644 index 0000000000..cceca3a5e8 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-3/expected.js @@ -0,0 +1,18 @@ +var _ref =

Parent

; + +var _ref2 =
child
; + +(function () { + class App extends React.Component { + render() { + return
+ {_ref} + +
; + } + } + + const AppItem = () => { + return _ref2; + }; +}); diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-4/actual.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-4/actual.js new file mode 100644 index 0000000000..4bb8f9b561 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-4/actual.js @@ -0,0 +1,16 @@ +(function () { + const AppItem = () => { + return
child
; + }; + + class App extends React.Component { + render() { + return ( +
+

Parent

+ +
+ ); + } + } +}); diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-4/expected.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-4/expected.js new file mode 100644 index 0000000000..1a6411bc82 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-4/expected.js @@ -0,0 +1,20 @@ +var _ref =
child
; + +var _ref3 =

Parent

; + +(function () { + const AppItem = () => { + return _ref; + }; + + var _ref2 =
+ {_ref3} + +
; + + class App extends React.Component { + render() { + return _ref2; + } + } +}); diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope/actual.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope/actual.js new file mode 100644 index 0000000000..1727938441 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope/actual.js @@ -0,0 +1,14 @@ +export default class App extends React.Component { + render() { + return ( +
+

Parent

+ +
+ ); + } +} + +const AppItem = () => { + return
child
; +}; diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope/expected.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope/expected.js new file mode 100644 index 0000000000..5fc8260845 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope/expected.js @@ -0,0 +1,16 @@ +var _ref =

Parent

; + +export default class App extends React.Component { + render() { + return
+ {_ref} + +
; + } +} + +var _ref2 =
child
; + +const AppItem = () => { + return _ref2; +}; diff --git a/packages/babel-traverse/src/path/lib/hoister.js b/packages/babel-traverse/src/path/lib/hoister.js index 075333aea6..f218eaf2ad 100644 --- a/packages/babel-traverse/src/path/lib/hoister.js +++ b/packages/babel-traverse/src/path/lib/hoister.js @@ -61,6 +61,36 @@ export default class PathHoister { } getAttachmentPath() { + let path = this._getAttachmentPath(); + if (!path) return; + + let targetScope = path.scope; + + // don't allow paths that have their own lexical environments to pollute + if (targetScope.path === path) { + targetScope = path.scope.parent; + } + + // avoid hoisting to a scope that contains bindings that are executed after our attachment path + if (targetScope.path.isProgram() || targetScope.path.isFunction()) { + for (let name in this.bindings) { + // check binding is a direct child of this paths scope + if (!targetScope.hasOwnBinding(name)) continue; + + let binding = this.bindings[name]; + + // allow parameter references + 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; + } + } + + return path; + } + + _getAttachmentPath() { let scopes = this.scopes; let scope = scopes.pop(); @@ -112,8 +142,8 @@ export default class PathHoister { // don't bother hoisting to the same function as this will cause multiple branches to be evaluated more than once leading to a bad optimisation if (attachTo.getFunctionParent() === this.path.getFunctionParent()) return; + // generate declaration and insert it to our point let uid = attachTo.scope.generateUidIdentifier("ref"); - attachTo.insertBefore([ t.variableDeclaration("var", [ t.variableDeclarator(uid, this.path.node) @@ -121,7 +151,6 @@ export default class PathHoister { ]); let parent = this.path.parentPath; - if (parent.isJSXElement() && this.path.container === parent.node.children) { // turning the `span` in `
` to an expression so we need to wrap it with // an expression container