Skip to content

Commit 300e477

Browse files
nicolo-ribaudonicolo-ribaudo
authored
allowMutablePropsOnTags: cache JSX const elements w/ fn props (#12975)
1 parent 2de3f1c commit 300e477

26 files changed

Lines changed: 157 additions & 116 deletions

File tree

‎packages/babel-plugin-transform-react-constant-elements/src/index.ts‎

Lines changed: 66 additions & 111 deletions
Original file line numberDiff line numberDiff line change
@@ -41,17 +41,18 @@ export default declare((api, options) => {
4141
return scope;
4242
}
4343

44-
const analyzer = {
44+
const immutabilityVisitor = {
4545
enter(path, state) {
4646
const stop = () => {
4747
state.isImmutable = false;
4848
path.stop();
4949
};
5050

51-
if (path.isJSXClosingElement()) {
51+
const skip = () => {
5252
path.skip();
53-
return;
54-
}
53+
};
54+
55+
if (path.isJSXClosingElement()) return skip();
5556

5657
// Elements with refs are not safe to hoist.
5758
if (
@@ -61,54 +62,70 @@ export default declare((api, options) => {
6162
return stop();
6263
}
6364

64-
// Ignore identifiers & JSX expressions.
65+
// Ignore JSX expressions and immutable values.
6566
if (
6667
path.isJSXIdentifier() ||
6768
path.isJSXMemberExpression() ||
68-
path.isJSXNamespacedName()
69+
path.isJSXNamespacedName() ||
70+
path.isImmutable()
6971
) {
7072
return;
7173
}
7274

75+
// Ignore constant bindings.
7376
if (path.isIdentifier()) {
7477
const binding = path.scope.getBinding(path.node.name);
7578
if (binding && binding.constant) return;
7679
}
7780

78-
if (!path.isImmutable()) {
79-
// If it's not immutable, it may still be a pure expression, such as string concatenation.
80-
// It is still safe to hoist that, so long as its result is immutable.
81-
// If not, it is not safe to replace as mutable values (like objects) could be mutated after render.
82-
// https://github.com/facebook/react/issues/3226
83-
if (path.isPure()) {
84-
const expressionResult = path.evaluate();
85-
if (expressionResult.confident) {
86-
// We know the result; check its mutability.
87-
const { value } = expressionResult;
88-
const isMutable =
89-
(!state.mutablePropsAllowed &&
90-
value &&
91-
typeof value === "object") ||
92-
typeof value === "function";
93-
if (!isMutable) {
94-
// It evaluated to an immutable value, so we can hoist it.
95-
path.skip();
96-
return;
97-
}
98-
} else if (t.isIdentifier(expressionResult.deopt)) {
99-
// It's safe to hoist here if the deopt reason is an identifier (e.g. func param).
100-
// The hoister will take care of how high up it can be hoisted.
101-
return;
102-
}
81+
// If we allow mutable props, tags with function expressions can be
82+
// safely hoisted.
83+
const { mutablePropsAllowed } = state;
84+
if (mutablePropsAllowed && path.isFunction()) {
85+
path.traverse(targetScopeVisitor, state);
86+
return skip();
87+
}
88+
89+
if (!path.isPure()) return stop();
90+
91+
// If it's not immutable, it may still be a pure expression, such as string concatenation.
92+
// It is still safe to hoist that, so long as its result is immutable.
93+
// If not, it is not safe to replace as mutable values (like objects) could be mutated after render.
94+
// https://github.com/facebook/react/issues/3226
95+
const expressionResult = path.evaluate();
96+
if (expressionResult.confident) {
97+
// We know the result; check its mutability.
98+
const { value } = expressionResult;
99+
if (
100+
mutablePropsAllowed ||
101+
value === null ||
102+
(typeof value !== "object" && typeof value !== "function")
103+
) {
104+
// It evaluated to an immutable value, so we can hoist it.
105+
return skip();
103106
}
104-
stop();
107+
} else if (t.isIdentifier(expressionResult.deopt)) {
108+
// It's safe to hoist here if the deopt reason is an identifier (e.g. func param).
109+
// The hoister will take care of how high up it can be hoisted.
110+
return;
105111
}
112+
113+
stop();
106114
},
115+
};
107116

117+
const targetScopeVisitor = {
108118
ReferencedIdentifier(path, state) {
109119
const { node } = path;
110120
let { scope } = path;
111121

122+
while (scope !== state.jsxScope) {
123+
// If a binding is declared in an inner function, it doesn't affect hoisting.
124+
if (declares(node, scope)) return;
125+
126+
scope = scope.parent;
127+
}
128+
112129
while (scope) {
113130
// We cannot hoist outside of the previous hoisting target
114131
// scope, so we return early and we don't update it.
@@ -124,72 +141,13 @@ export default declare((api, options) => {
124141

125142
state.targetScope = getHoistingScope(scope);
126143
},
127-
128-
/*
129-
See the discussion at https://github.com/babel/babel/pull/12967#discussion_r587948958
130-
to uncomment this code.
131-
132-
ReferencedIdentifier(path, state) {
133-
const { node } = path;
134-
let { scope } = path;
135-
let targetScope;
136-
137-
let isNestedScope = true;
138-
let needsHoisting = true;
139-
140-
while (scope) {
141-
// We cannot hoist outside of the previous hoisting target
142-
// scope, so we return early and we don't update it.
143-
if (scope === state.targetScope) return;
144-
145-
// When we hit the scope of our JSX element, we must start
146-
// checking if they declare the binding of the current
147-
// ReferencedIdentifier.
148-
// We don't case about bindings declared in nested scopes,
149-
// because the whole nested scope is hoisted alongside the
150-
// JSX element so it doesn't impose any extra constraint.
151-
if (scope === state.jsxScope) {
152-
isNestedScope = false;
153-
}
154-
155-
// If we are in an upper scope and hoisting to this scope has
156-
// any benefit, we update the possible targetScope to the
157-
// current one.
158-
if (!isNestedScope && needsHoisting) {
159-
targetScope = scope;
160-
}
161-
162-
// When we start walking in upper scopes, avoid hoisting JSX
163-
// elements until we hit a scope introduced by a function or
164-
// loop.
165-
// This is because hoisting from the inside to the outside
166-
// of block or if statements doesn't give any performance
167-
// benefit, and it just unnecessarily increases the code size.
168-
if (scope === state.jsxScope) {
169-
needsHoisting = false;
170-
}
171-
if (!needsHoisting && isHoistingScope(scope)) {
172-
needsHoisting = true;
173-
}
174-
175-
// If the current scope declares the ReferencedIdentifier we
176-
// are checking, we break out of this loop. There are two
177-
// possible scenarios:
178-
// 1. We are in a nested scope, this this declaration means
179-
// that this reference doesn't affect the target scope.
180-
// The targetScope variable is still undefined.
181-
// 2. We are in an upper scope, so this declaration defines
182-
// a new hoisting constraint. The targetScope variable
183-
// refers to the current scope.
184-
if (declares(node, scope)) break;
185-
186-
scope = scope.parent;
187-
}
188-
189-
if (targetScope) state.targetScope = targetScope;
190-
},*/
191144
};
192145

146+
// We cannot use traverse.visitors.merge because it doesn't support
147+
// immutabilityVisitor's bare `enter` visitor.
148+
// It's safe to just use ... because the two visitors don't share any key.
149+
const hoistingVisitor = { ...immutabilityVisitor, ...targetScopeVisitor };
150+
193151
return {
194152
name: "transform-react-constant-elements",
195153

@@ -216,21 +174,6 @@ export default declare((api, options) => {
216174
mutablePropsAllowed = allowMutablePropsOnTags.includes(elementName);
217175
}
218176

219-
const state = {
220-
isImmutable: true,
221-
mutablePropsAllowed,
222-
targetScope: path.scope.getProgramParent(),
223-
};
224-
225-
// Traverse all props passed to this element for immutability,
226-
// and compute the target hoisting scope
227-
path.traverse(analyzer, state);
228-
229-
if (!state.isImmutable) return;
230-
231-
const { targetScope } = state;
232-
HOISTED.set(path.node, targetScope);
233-
234177
// In order to avoid hoisting unnecessarily, we need to know which is
235178
// the scope containing the current JSX element. If a parent of the
236179
// current element has already been hoisted, we can consider its target
@@ -243,6 +186,18 @@ export default declare((api, options) => {
243186
}
244187
jsxScope ??= getHoistingScope(path.scope);
245188

189+
const visitorState = {
190+
isImmutable: true,
191+
mutablePropsAllowed,
192+
jsxScope,
193+
targetScope: path.scope.getProgramParent(),
194+
};
195+
path.traverse(hoistingVisitor, visitorState);
196+
if (!visitorState.isImmutable) return;
197+
198+
const { targetScope } = visitorState;
199+
HOISTED.set(path.node, targetScope);
200+
246201
// Only hoist if it would give us an advantage.
247202
if (targetScope === jsxScope) return;
248203

packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist/input.js renamed to packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/basic/input.js

File renamed without changes.

packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist-member/options.json renamed to packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/basic/options.json

File renamed without changes.

packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist/output.js renamed to packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/basic/output.js

File renamed without changes.

‎packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/input.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
function Component({ increment }) {
2+
return () => <Counter onClick={value => value + increment} />;
3+
}

‎packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/options.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"plugins": [
3+
[
4+
"transform-react-constant-elements",
5+
{ "allowMutablePropsOnTags": ["Counter"] }
6+
],
7+
"syntax-jsx"
8+
]
9+
}

‎packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/output.js‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
function Component({
2+
increment
3+
}) {
4+
var _Counter;
5+
6+
return () => _Counter || (_Counter = <Counter onClick={value => value + increment} />);
7+
}

‎packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/input.js‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
function Component() {
2+
return () => <Counter onClick={value => value + prompt("Increment:")} />;
3+
}

‎packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/options.json‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
{
2+
"plugins": [
3+
[
4+
"transform-react-constant-elements",
5+
{ "allowMutablePropsOnTags": ["Counter"] }
6+
],
7+
"syntax-jsx"
8+
]
9+
}

‎packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/output.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
var _Counter;
2+
3+
function Component() {
4+
return () => _Counter || (_Counter = <Counter onClick={value => value + prompt("Increment:")} />);
5+
}

0 commit comments

Comments
 (0)