cerxes/babel
3
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

Fix PathHoister hoisting before bindings. (#5153)

Browse Source 
Fixes #5149 and enables a few additional safe hoists.
This commit is contained in:
Samuel Reed
2017-02-13 09:35:08 +07:00
committed by Logan Smyth
parent eb91bd831c
commit f4e3dfee74
8 changed files with 125 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope-3/expected.js vendored
View File

@@ -1,18 +1,19 @@
var _ref = <p>Parent</p>;
var _ref2 = <div>child</div>;
var _ref3 = <p>Parent</p>;
(function () {
class App extends React.Component {
render() {
return <div>
{_ref}
<AppItem />
</div>;
return _ref;
}
}
const AppItem = () => {
return _ref2;
};
});
},
_ref = <div>
{_ref3}
<AppItem />
</div>;
});

18
packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/append-to-end-when-declared-in-scope/expected.js vendored
View File

@@ -1,16 +1,14 @@
var _ref = <p>Parent</p>;
export default class App extends React.Component {
render() {
return <div>
{_ref}
<AppItem />
</div>;
return _ref;
}
}
var _ref2 = <div>child</div>;
const AppItem = () => {
const _ref2 = <div>child</div>,
AppItem = () => {
return _ref2;
};
},
_ref = <div>
<p>Parent</p>
<AppItem />
</div>;

15
packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-before-class/actual.js vendored Normal file
View File

@@ -0,0 +1,15 @@
import React from "react";
const Parent = ({}) => (
<div className="parent">
<Child/>
</div>
);
export default Parent;
let Child = () => (
<div className="child">
ChildTextContent
</div>
);

13
packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-before-class/expected.js vendored Normal file
View File

@@ -0,0 +1,13 @@
import React from "react";
const Parent = ({}) => _ref;
export default Parent;
let _ref2 = <div className="child">
ChildTextContent
</div>,
Child = () => _ref2,
_ref = <div className="parent">
<Child />
</div>;

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

@@ -8,8 +8,9 @@ function render() {
function render() {
const bar = "bar",
renderFoo = () => <foo bar={bar} baz={baz} />,
baz = "baz";
renderFoo = () => _ref2,
baz = "baz",
_ref2 = <foo bar={bar} baz={baz} />;
return renderFoo();
}

18
packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-before-hoc/actual.js vendored Normal file
View File

@@ -0,0 +1,18 @@
import React from "react";
const HOC = component => component;
const Parent = ({}) => (
<div className="parent">
<Child/>
</div>
);
export default Parent;
let Child = () => (
<div className="child">
ChildTextContent
</div>
);
Child = HOC(Child);

18
packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/dont-hoist-before-hoc/expected.js vendored Normal file
View File

@@ -0,0 +1,18 @@
import React from "react";
const HOC = component => component;
const Parent = ({}) => _ref;
export default Parent;
var _ref2 = <div className="child">
ChildTextContent
</div>;
let Child = () => _ref2;
Child = HOC(Child);
var _ref = <div className="parent">
<Child />
</div>;

62
packages/babel-traverse/src/path/lib/hoister.js
View File

@@ -27,25 +27,27 @@ const referenceVisitor = {
// eg. it's in a closure etc
if (binding !== state.scope.getBinding(path.node.name)) return;
if (binding.constant) {
state.bindings[path.node.name] = binding;
} else {
for (const violationPath of (binding.constantViolations: Array)) {
state.breakOnScopePaths = state.breakOnScopePaths.concat(violationPath.getAncestry());
}
}
state.bindings[path.node.name] = binding;
}
};
export default class PathHoister {
constructor(path, scope) {
// Storage for scopes we can't hoist above.
this.breakOnScopePaths = [];
// Storage for bindings that may affect what path we can hoist to.
this.bindings = {};
// Storage for eligible scopes.
this.scopes = [];
// Our original scope and path.
this.scope = scope;
this.path = path;
// By default, we attach as far up as we can; but if we're trying
// to avoid referencing a binding, we may have to go after.
this.attachAfter = false;
}
// A scope is compatible if all required bindings are reachable.
isCompatibleScope(scope) {
for (const key in this.bindings) {
const binding = this.bindings[key];
@@ -57,6 +59,7 @@ export default class PathHoister {
return true;
}
// Look through all scopes and push compatible ones.
getCompatibleScopes() {
let scope = this.path.scope;
do {
@@ -66,6 +69,7 @@ export default class PathHoister {
break;
}
// deopt: These scopes are set in the visitor on const violations
if (this.breakOnScopePaths.indexOf(scope.path) >= 0) {
break;
}
@@ -73,7 +77,7 @@ export default class PathHoister {
}
getAttachmentPath() {
const path = this._getAttachmentPath();
let path = this._getAttachmentPath();
if (!path) return;
let targetScope = path.scope;
@@ -94,8 +98,18 @@ export default class PathHoister {
// allow parameter references
if (binding.kind === "param") continue;
// if this binding appears after our attachment point then don't hoist it
if (this.getAttachmentParentForPath(binding.path).key > path.key) return;
// if this binding appears after our attachment point, then we move after it.
if (this.getAttachmentParentForPath(binding.path).key > path.key) {
this.attachAfter = true;
path = binding.path;
// We also move past any constant violations.
for (const violationPath of (binding.constantViolations: Array)) {
if (this.getAttachmentParentForPath(violationPath).key > path.key) {
path = violationPath;
}
}
}
}
}
@@ -106,11 +120,12 @@ export default class PathHoister {
const scopes = this.scopes;
const scope = scopes.pop();
// deopt: no compatible scopes
if (!scope) return;
if (scope.path.isFunction()) {
if (this.hasOwnParamBindings(scope)) {
// should ignore this scope since it's ourselves
// deopt: should ignore this scope since it's ourselves
if (this.scope === scope) return;
// needs to be attached to the body
@@ -129,26 +144,30 @@ export default class PathHoister {
if (scope) return this.getAttachmentParentForPath(scope.path);
}
// Find an attachment for this path.
getAttachmentParentForPath(path) {
do {
if (!path.parentPath ||
(Array.isArray(path.container) && path.isStatement()) ||
(
path.isVariableDeclarator() &&
path.parentPath.node !== null &&
path.parentPath.node.declarations.length > 1
)
)
if (
// Beginning of the scope
!path.parentPath ||
// Has siblings and is a statement
(Array.isArray(path.container) && path.isStatement()) ||
// Is part of multiple var declarations
(path.isVariableDeclarator() &&
path.parentPath.node !== null &&
path.parentPath.node.declarations.length > 1))
return path;
} while ((path = path.parentPath));
}
// Returns true if a scope has param bindings.
hasOwnParamBindings(scope) {
for (const name in this.bindings) {
if (!scope.hasOwnBinding(name)) continue;
const binding = this.bindings[name];
if (binding.kind === "param") return true;
// Ensure constant; without it we could place behind a reassignment
if (binding.kind === "param" && binding.constant) return true;
}
return false;
}
@@ -173,7 +192,8 @@ export default class PathHoister {
let uid = attachTo.scope.generateUidIdentifier("ref");
const declarator = t.variableDeclarator(uid, this.path.node);
attachTo.insertBefore([
const insertFn = this.attachAfter ? "insertAfter" : "insertBefore";
attachTo[insertFn]([
attachTo.isVariableDeclarator() ? declarator : t.variableDeclaration("var", [declarator])
]);