New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Lazily initialize and cache constant JSX elements #12967
Changes from 5 commits
06c8964
be0bef8
10c5b08
987948f
6197d97
294a08b
e98bff1
6c20164
7de9a22
f56c1ef
e239247
a4e8550
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,16 @@ | ||
var _Contact; | ||
|
||
const title = "Neem contact op"; | ||
|
||
function action() { | ||
return _action.apply(this, arguments); | ||
} | ||
|
||
var _ref = /*#__PURE__*/React.createElement(Contact, { | ||
title: title | ||
}); | ||
|
||
function _action() { | ||
_action = babelHelpers.asyncToGenerator(function* () { | ||
return _ref; | ||
return _Contact || (_Contact = /*#__PURE__*/React.createElement(Contact, { | ||
title: title | ||
})); | ||
}); | ||
return _action.apply(this, arguments); | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -1,5 +1,5 @@ | ||||||
import { declare } from "@babel/helper-plugin-utils"; | ||||||
import { types as t } from "@babel/core"; | ||||||
import { types as t, template } from "@babel/core"; | ||||||
|
||||||
export default declare((api, options) => { | ||||||
api.assertVersion(7); | ||||||
|
@@ -15,9 +15,10 @@ export default declare((api, options) => { | |||||
); | ||||||
} | ||||||
|
||||||
const HOISTED = new WeakSet(); | ||||||
// Element -> Target scope | ||||||
const HOISTED = new WeakMap(); | ||||||
|
||||||
const immutabilityVisitor = { | ||||||
const analyzer = { | ||||||
enter(path, state) { | ||||||
const stop = () => { | ||||||
state.isImmutable = false; | ||||||
|
@@ -75,6 +76,68 @@ export default declare((api, options) => { | |||||
stop(); | ||||||
} | ||||||
}, | ||||||
ReferencedIdentifier(path, state) { | ||||||
const { name } = path.node; | ||||||
let scope = path.scope; | ||||||
let targetScope; | ||||||
|
||||||
let isNestedScope = true; | ||||||
let needsHoisting = true; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can actually reduce this code a bit: diff --git a/packages/babel-plugin-transform-react-constant-elements/src/index.js b/packages/babel-plugin-transform-react-constant-elements/src/index.js
index 40d929e992..393ea016c6 100644
--- a/packages/babel-plugin-transform-react-constant-elements/src/index.js
+++ b/packages/babel-plugin-transform-react-constant-elements/src/index.js
@@ -31,6 +31,17 @@ export default declare((api, options) => {
return scope.hasOwnBinding(node.name);
}
+ function getHoistingScope(scope) {
+ for (;;) {
+ const { path } = scope;
+ if (path.isFunctionParent() || path.isLoop()) {
+ break;
+ }
+ scope = scope.parent;
+ }
+ return scope;
+ }
+
const analyzer = {
enter(path, state) {
const stop = () => {
@@ -89,67 +100,25 @@ export default declare((api, options) => {
stop();
}
},
- "ReferencedIdentifier|ThisExpression"(path, state) {
+
+ ReferencedIdentifier(path, state) {
const { node } = path;
let { scope } = path;
- let targetScope;
-
- let isNestedScope = true;
- let needsHoisting = true;
while (scope) {
// We cannot hoist outside of the previous hoisting target
// scope, so we return early and we don't update it.
if (scope === state.targetScope) return;
- // When we hit the scope of our JSX element, we must start
- // checking if they declare the binding of the current
- // ReferencedIdentifier.
- // We don't case about bindings declared in nested scopes,
- // because the whole nested scope is hoisted alongside the
- // JSX element so it doesn't impose any extra constraint.
- if (scope === state.jsxScope) {
- isNestedScope = false;
- }
-
- // If we are in an upper scope and hoisting to this scope has
- // any benefit, we update the possible targetScope to the
- // current one.
- if (!isNestedScope && needsHoisting) {
- targetScope = scope;
- }
-
- // When we start walking in upper scopes, avoid hoisting JSX
- // elements until we hit a scope introduced by a function or
- // loop.
- // This is because hoisting from the inside to the outside
- // of block or if statements doesn't give any performance
- // benefit, and it just unnecessarily increases the code size.
- if (scope === state.jsxScope) {
- needsHoisting = false;
- }
- if (
- !needsHoisting &&
- (scope.path.isFunctionParent() || scope.path.isLoop())
- ) {
- needsHoisting = true;
- }
-
- // If the current scope declares the ReferencedIdentifier we
- // are checking, we break out of this loop. There are two
- // possible scenarios:
- // 1. We are in a nested scope, this this declaration means
- // that this reference doesn't affect the target scope.
- // The targetScope variable is still undefined.
- // 2. We are in an upper scope, so this declaration defines
- // a new hoisting constraint. The targetScope variable
- // refers to the current scope.
+ // If the scope declares this identifier (or we're at the function
+ // providing the lexical env binding), we can't hoist the var any
+ // higher.
if (declares(node, scope)) break;
scope = scope.parent;
}
- if (targetScope) state.targetScope = targetScope;
+ state.targetScope = getHoistingScope(scope);
},
};
@@ -179,23 +148,10 @@ export default declare((api, options) => {
mutablePropsAllowed = allowMutablePropsOnTags.includes(elementName);
}
- // In order to avoid hoisting unnecessarily, we need to know which is
- // the scope containing the current JSX element. If a parent of the
- // current element has already been hoisted, we can consider its target
- // scope as the base scope for the current element.
- let jsxScope;
- let current = path;
- while (!jsxScope && current.parentPath.isJSXElement()) {
- current = current.parentPath;
- jsxScope = HOISTED.get(current.node);
- }
- jsxScope ??= path.scope;
-
const state = {
isImmutable: true,
mutablePropsAllowed,
- jsxScope,
- targetScope: null,
+ targetScope: path.scope.getProgramParent(),
};
// Traverse all props passed to this element for immutability,
@@ -206,9 +162,21 @@ export default declare((api, options) => {
// If we didn't find any hoisting constraint, we can hoist the current
// helement to the program scope.
- const targetScope = state.targetScope ?? path.scope.getProgramParent();
+ const { targetScope } = state;
HOISTED.set(path.node, targetScope);
+ // In order to avoid hoisting unnecessarily, we need to know which is
+ // the scope containing the current JSX element. If a parent of the
+ // current element has already been hoisted, we can consider its target
+ // scope as the base scope for the current element.
+ let jsxScope;
+ let current = path;
+ while (!jsxScope && current.parentPath.isJSXElement()) {
+ current = current.parentPath;
+ jsxScope = HOISTED.get(current.node);
+ }
+ jsxScope ??= getHoistingScope(path.scope);
+
// Only hoist if it would give us an advantage.
if (targetScope === jsxScope) return;
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seems to unnecessarily cache some components: function Component({ isLoading, text }) {
if (isLoading) {
return <p>Loading {text}</p>;
}
return <p>Loaded {text}</p>;
} function Component({
isLoading,
text
}) {
var _p;
if (isLoading) {
return _p || (_p = <p>Loading {text}</p>);
}
return <p>Loaded {text}</p>;
} which is the reason I introduced the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I edited the diff a few hours after initially comment, are you sure you're using the latest? You can see it running at https://astexplorer.net/#/gist/15ad6fa36177a1ba8f06a94c7dd28f8a/bedfd7d67d72dbb98b33be1e66f98785c3b18aac. This test case (and all the others) are passing for me. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I probably made some mistake while applying your diff. I'm just commenting out my code instead of deleting it, since it will be needed if/when we do #12967 (comment) for cases like function Component({ isLoading, text }) {
return () => <p>Loading {text => text}</p>;
} which with the simplified logic would become function Component({
isLoading,
text
}) {
return () => _p || (_p = <p>Loading {text => {
var _p;
return text;
}}</p>);
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we'll be able to handle that case with the simplified code, but let's wait for the followup PR. |
||||||
|
||||||
while (scope) { | ||||||
// We cannot hoist outside of the previous hoisting target | ||||||
// scope, so we return early and we don't update it. | ||||||
if (scope === state.targetScope) return; | ||||||
|
||||||
// When we hit the scope of our JSX element, we must start | ||||||
// checking if they declare the binding of the current | ||||||
// ReferencedIdentifier. | ||||||
// We don't case about bindings declared in nested scopes, | ||||||
// because the whole nested scope is hoisted alongside the | ||||||
// JSX element so it doesn't impose any extra constraint. | ||||||
if (scope === state.jsxScope) { | ||||||
isNestedScope = false; | ||||||
} | ||||||
|
||||||
// If we are in an upper scope and hoisting to this scope has | ||||||
// any benefit, we update the possible targetScope to the | ||||||
// current one. | ||||||
if (!isNestedScope && needsHoisting) { | ||||||
targetScope = scope; | ||||||
} | ||||||
|
||||||
// When we start walking in upper scopes, avoid hoisting JSX | ||||||
// elements until we hit a scope introduced by a function or | ||||||
// loop. | ||||||
// This is because hoisting from the inside to the outside | ||||||
// of block or if statements doesn't give any performance | ||||||
// benefit, and it just unnecessarily increases the code size. | ||||||
if (scope === state.jsxScope) { | ||||||
needsHoisting = false; | ||||||
} | ||||||
if ( | ||||||
!needsHoisting && | ||||||
(scope.path.isFunctionParent() || scope.path.isLoop()) | ||||||
) { | ||||||
needsHoisting = true; | ||||||
} | ||||||
|
||||||
// If the current scope declares the ReferencedIdentifier we | ||||||
// are checking, we break out of this loop. There are two | ||||||
// possible scenarios: | ||||||
// 1. We are in a nested scope, this this declaration means | ||||||
// that this reference doesn't affect the target scope. | ||||||
// The targetScope variable is still undefined. | ||||||
// 2. We are in an upper scope, so this declaration defines | ||||||
// a new hoisting constraint. The targetScope variable | ||||||
// refers to the current scope. | ||||||
if (scope.hasOwnBinding(name)) break; | ||||||
|
||||||
scope = scope.parent; | ||||||
} | ||||||
|
||||||
if (targetScope) state.targetScope = targetScope; | ||||||
}, | ||||||
}; | ||||||
|
||||||
return { | ||||||
|
@@ -83,30 +146,70 @@ export default declare((api, options) => { | |||||
visitor: { | ||||||
JSXElement(path) { | ||||||
if (HOISTED.has(path.node)) return; | ||||||
HOISTED.add(path.node); | ||||||
HOISTED.set(path.node, path.scope); | ||||||
|
||||||
const state = { isImmutable: true }; | ||||||
const name = path.get("openingElement.name").node; | ||||||
nicolo-ribaudo marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
||||||
// This transform takes the option `allowMutablePropsOnTags`, which is an array | ||||||
// of JSX tags to allow mutable props (such as objects, functions) on. Use sparingly | ||||||
// and only on tags you know will never modify their own props. | ||||||
let mutablePropsAllowed = false; | ||||||
if (allowMutablePropsOnTags != null) { | ||||||
// Get the element's name. If it's a member expression, we use the last part of the path. | ||||||
// So the option ["FormattedMessage"] would match "Intl.FormattedMessage". | ||||||
let namePath = path.get("openingElement.name"); | ||||||
while (namePath.isJSXMemberExpression()) { | ||||||
namePath = namePath.get("property"); | ||||||
let lastSegment = name; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Can you add a test case like function AComponent () {
return <BComponent/>
function BComponent () {
return <n:CComponent />
}
}
|
||||||
while (t.isJSXMemberExpression(lastSegment)) { | ||||||
lastSegment = lastSegment.property; | ||||||
} | ||||||
|
||||||
const elementName = namePath.node.name; | ||||||
state.mutablePropsAllowed = | ||||||
allowMutablePropsOnTags.indexOf(elementName) > -1; | ||||||
const elementName = lastSegment.name; | ||||||
mutablePropsAllowed = allowMutablePropsOnTags.includes(elementName); | ||||||
} | ||||||
|
||||||
// Traverse all props passed to this element for immutability. | ||||||
path.traverse(immutabilityVisitor, state); | ||||||
// In order to avoid hoisting unnecessarily, we need to know which is | ||||||
// the scope containing the current JSX element. If a parent of the | ||||||
// current element has already been hoisted, we can consider its target | ||||||
// scope as the base scope for the current element. | ||||||
jridgewell marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
let jsxScope; | ||||||
let current = path; | ||||||
while (!jsxScope && current.parentPath.isJSXElement()) { | ||||||
current = current.parentPath; | ||||||
jsxScope = HOISTED.get(current.node); | ||||||
} | ||||||
jsxScope ??= path.scope; | ||||||
|
||||||
const state = { | ||||||
isImmutable: true, | ||||||
mutablePropsAllowed, | ||||||
jsxScope, | ||||||
targetScope: null, | ||||||
}; | ||||||
|
||||||
// Traverse all props passed to this element for immutability, | ||||||
// and compute the target hoisting scope | ||||||
path.traverse(analyzer, state); | ||||||
|
||||||
if (!state.isImmutable) return; | ||||||
|
||||||
// If we didn't find any hoisting constraint, we can hoist the current | ||||||
// helement to the program scope. | ||||||
const targetScope = state.targetScope ?? path.scope.getProgramParent(); | ||||||
HOISTED.set(path.node, targetScope); | ||||||
|
||||||
// Only hoist if it would give us an advantage. | ||||||
if (targetScope === jsxScope) return; | ||||||
|
||||||
const id = path.scope.generateUidBasedOnNode(name); | ||||||
targetScope.push({ id: t.identifier(id) }); | ||||||
|
||||||
let replacement = template.expression.ast` | ||||||
${t.identifier(id)} || (${t.identifier(id)} = ${path.node}) | ||||||
`; | ||||||
if (path.parentPath.isJSXElement()) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Test Case: function C(x) {
// Bind the div in this scope, so only span is hoisted.
return <div x={x} attr=<span /> />;
} |
||||||
replacement = t.jsxExpressionContainer(replacement); | ||||||
} | ||||||
|
||||||
if (state.isImmutable) path.hoist(); | ||||||
path.replaceWith(replacement); | ||||||
}, | ||||||
}, | ||||||
}; | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
function AComponent () { | ||
const CComponent = () => <div/> | ||
return <BComponent/> | ||
|
||
function BComponent () { | ||
return <CComponent/> | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
var _div; | ||
|
||
function AComponent() { | ||
var _CComponent; | ||
|
||
const CComponent = () => _div || (_div = <div />); | ||
|
||
return <BComponent />; | ||
|
||
function BComponent() { | ||
return _CComponent || (_CComponent = <CComponent />); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,15 @@ | ||
var _ref = <div>child</div>; | ||
var _div, _div2; | ||
|
||
const AppItem = () => { | ||
return _ref; | ||
return _div || (_div = <div>child</div>); | ||
}; | ||
|
||
var _ref2 = <div> | ||
<p>Parent</p> | ||
<AppItem /> | ||
</div>; | ||
|
||
export default class App extends React.Component { | ||
render() { | ||
return _ref2; | ||
return _div2 || (_div2 = <div> | ||
<p>Parent</p> | ||
<AppItem /> | ||
</div>); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,20 +1,19 @@ | ||
var _ref2 = <div>child</div>; | ||
|
||
var _ref3 = <p>Parent</p>; | ||
var _p, _div2; | ||
|
||
(function () { | ||
var _div; | ||
|
||
class App extends React.Component { | ||
render() { | ||
return _ref; | ||
return _div || (_div = <div> | ||
{_p || (_p = <p>Parent</p>)} | ||
<AppItem /> | ||
</div>); | ||
} | ||
|
||
} | ||
|
||
const AppItem = () => { | ||
return _ref2; | ||
}, | ||
_ref = <div> | ||
{_ref3} | ||
<AppItem /> | ||
</div>; | ||
return _div2 || (_div2 = <div>child</div>); | ||
}; | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,15 @@ | ||
var _div, _div2; | ||
|
||
export default class App extends React.Component { | ||
render() { | ||
return _ref; | ||
return _div || (_div = <div> | ||
<p>Parent</p> | ||
<AppItem /> | ||
</div>); | ||
} | ||
|
||
} | ||
|
||
var _ref2 = <div>child</div>; | ||
|
||
const AppItem = () => { | ||
return _ref2; | ||
}, | ||
_ref = <div> | ||
<p>Parent</p> | ||
<AppItem /> | ||
</div>; | ||
return _div2 || (_div2 = <div>child</div>); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,9 @@ | ||
var _ref = <span />; | ||
var _span; | ||
|
||
var Foo = React.createClass({ | ||
render: function () { | ||
return <div className={this.props.className}> | ||
{_ref} | ||
{_span || (_span = <span />)} | ||
</div>; | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,7 @@ | ||
var _Loader, _Loader2; | ||
|
||
import React from 'react'; | ||
import Loader from 'loader'; | ||
|
||
var _ref = <Loader className="full-height" />; | ||
|
||
var _ref2 = <Loader className="p-y-5" />; | ||
|
||
const errorComesHere = () => _ref, | ||
thisWorksFine = () => _ref2; | ||
const errorComesHere = () => _Loader || (_Loader = <Loader className="full-height" />), | ||
thisWorksFine = () => _Loader2 || (_Loader2 = <Loader className="p-y-5" />); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
var Foo = require("Foo"); | ||
var _Foo; | ||
|
||
var _ref = <Foo />; | ||
var Foo = require("Foo"); | ||
|
||
function render() { | ||
return _ref; | ||
return _Foo || (_Foo = <Foo />); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Babel 8 we could just use
||=
here, and if someone targets an older browser it will be transformed by another different plugin.