From eb91bd831c05443e3ad34b83ad405b9d8137c09a Mon Sep 17 00:00:00 2001 From: Samuel Reed Date: Mon, 13 Feb 2017 09:34:07 +0700 Subject: [PATCH] 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. --- .../member-expression-constant/actual.js | 4 ++++ .../member-expression-constant/expected.js | 7 +++++++ .../member-expression-this/actual.js | 5 +++++ .../member-expression-this/expected.js | 12 ++++++++++++ .../member-expression-this/options.json | 3 +++ .../member-expression/actual.js | 6 ++++++ .../member-expression/expected.js | 16 ++++++++++++++++ .../member-expression/options.json | 3 +++ packages/babel-traverse/src/path/lib/hoister.js | 14 +++++++++++++- 9 files changed, 69 insertions(+), 1 deletion(-) create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-constant/actual.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-constant/expected.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/actual.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/expected.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/options.json create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/actual.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/expected.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/options.json diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-constant/actual.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-constant/actual.js new file mode 100644 index 0000000000..a6c1f9b6ec --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-constant/actual.js @@ -0,0 +1,4 @@ +function render() { + this.component = "div"; + return () => ; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-constant/expected.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-constant/expected.js new file mode 100644 index 0000000000..aa7a9994ba --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-constant/expected.js @@ -0,0 +1,7 @@ +function render() { + this.component = "div"; + + var _ref = ; + + return () => _ref; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/actual.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/actual.js new file mode 100644 index 0000000000..bdc61d684a --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/actual.js @@ -0,0 +1,5 @@ +class Component extends React.Component { + subComponent = () => Sub Component + + render = () => +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/expected.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/expected.js new file mode 100644 index 0000000000..9ce3a08e74 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/expected.js @@ -0,0 +1,12 @@ +var _ref = Sub Component; + +class Component extends React.Component { + constructor(...args) { + var _temp; + + var _ref2 = ; + + return _temp = super(...args), this.subComponent = () => _ref, this.render = () => _ref2, _temp; + } + +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/options.json b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/options.json new file mode 100644 index 0000000000..d4789bbda3 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression-this/options.json @@ -0,0 +1,3 @@ +{ + "plugins": ["syntax-jsx", "transform-react-constant-elements", "transform-class-properties"] +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/actual.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/actual.js new file mode 100644 index 0000000000..2ab59df97b --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/actual.js @@ -0,0 +1,6 @@ +const els = { + subComponent: () => Sub Component +}; +class Component extends React.Component { + render = () => +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/expected.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/expected.js new file mode 100644 index 0000000000..fe12eb988d --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/expected.js @@ -0,0 +1,16 @@ +var _ref = Sub Component; + +const els = { + subComponent: () => _ref +}; + +var _ref2 = ; + +class Component extends React.Component { + constructor(...args) { + var _temp; + + return _temp = super(...args), this.render = () => _ref2, _temp; + } + +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/options.json b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/options.json new file mode 100644 index 0000000000..d4789bbda3 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/member-expression/options.json @@ -0,0 +1,3 @@ +{ + "plugins": ["syntax-jsx", "transform-react-constant-elements", "transform-class-properties"] +} diff --git a/packages/babel-traverse/src/path/lib/hoister.js b/packages/babel-traverse/src/path/lib/hoister.js index 93b9323bd3..c7dbf5cf73 100644 --- a/packages/babel-traverse/src/path/lib/hoister.js +++ b/packages/babel-traverse/src/path/lib/hoister.js @@ -2,11 +2,23 @@ import { react } from "babel-types"; import * as t from "babel-types"; const referenceVisitor = { + // This visitor looks for bindings to establish a topmost scope for hoisting. ReferencedIdentifier(path, state) { - if (path.isJSXIdentifier() && react.isCompatTag(path.node.name)) { + // Don't hoist regular JSX identifiers ('div', 'span', etc). + // We do have to consider member expressions for hoisting (e.g. `this.component`) + if (path.isJSXIdentifier() && react.isCompatTag(path.node.name) && !path.parentPath.isJSXMemberExpression()) { return; } + // If the identifier refers to `this`, we need to break on the closest non-arrow scope. + if (path.node.name === "this") { + let scope = path.scope; + do { + if (scope.path.isFunction() && !scope.path.isArrowFunctionExpression()) break; + } while (scope = scope.parent); + if (scope) state.breakOnScopePaths.push(scope.path); + } + // direct references that we need to track to hoist this to the highest scope we can const binding = path.scope.getBinding(path.node.name); if (!binding) return;