diff --git a/packages/babel-plugin-transform-react-constant-elements/src/index.ts b/packages/babel-plugin-transform-react-constant-elements/src/index.ts index 2beafc4965dc..af44cba5d037 100644 --- a/packages/babel-plugin-transform-react-constant-elements/src/index.ts +++ b/packages/babel-plugin-transform-react-constant-elements/src/index.ts @@ -41,17 +41,18 @@ export default declare((api, options) => { return scope; } - const analyzer = { + const immutabilityVisitor = { enter(path, state) { const stop = () => { state.isImmutable = false; path.stop(); }; - if (path.isJSXClosingElement()) { + const skip = () => { path.skip(); - return; - } + }; + + if (path.isJSXClosingElement()) return skip(); // Elements with refs are not safe to hoist. if ( @@ -61,54 +62,70 @@ export default declare((api, options) => { return stop(); } - // Ignore identifiers & JSX expressions. + // Ignore JSX expressions and immutable values. if ( path.isJSXIdentifier() || path.isJSXMemberExpression() || - path.isJSXNamespacedName() + path.isJSXNamespacedName() || + path.isImmutable() ) { return; } + // Ignore constant bindings. if (path.isIdentifier()) { const binding = path.scope.getBinding(path.node.name); if (binding && binding.constant) return; } - if (!path.isImmutable()) { - // If it's not immutable, it may still be a pure expression, such as string concatenation. - // It is still safe to hoist that, so long as its result is immutable. - // If not, it is not safe to replace as mutable values (like objects) could be mutated after render. - // https://github.com/facebook/react/issues/3226 - if (path.isPure()) { - const expressionResult = path.evaluate(); - if (expressionResult.confident) { - // We know the result; check its mutability. - const { value } = expressionResult; - const isMutable = - (!state.mutablePropsAllowed && - value && - typeof value === "object") || - typeof value === "function"; - if (!isMutable) { - // It evaluated to an immutable value, so we can hoist it. - path.skip(); - return; - } - } else if (t.isIdentifier(expressionResult.deopt)) { - // It's safe to hoist here if the deopt reason is an identifier (e.g. func param). - // The hoister will take care of how high up it can be hoisted. - return; - } + // If we allow mutable props, tags with function expressions can be + // safely hoisted. + const { mutablePropsAllowed } = state; + if (mutablePropsAllowed && path.isFunction()) { + path.traverse(targetScopeVisitor, state); + return skip(); + } + + if (!path.isPure()) return stop(); + + // If it's not immutable, it may still be a pure expression, such as string concatenation. + // It is still safe to hoist that, so long as its result is immutable. + // If not, it is not safe to replace as mutable values (like objects) could be mutated after render. + // https://github.com/facebook/react/issues/3226 + const expressionResult = path.evaluate(); + if (expressionResult.confident) { + // We know the result; check its mutability. + const { value } = expressionResult; + if ( + mutablePropsAllowed || + value === null || + (typeof value !== "object" && typeof value !== "function") + ) { + // It evaluated to an immutable value, so we can hoist it. + return skip(); } - stop(); + } else if (t.isIdentifier(expressionResult.deopt)) { + // It's safe to hoist here if the deopt reason is an identifier (e.g. func param). + // The hoister will take care of how high up it can be hoisted. + return; } + + stop(); }, + }; + const targetScopeVisitor = { ReferencedIdentifier(path, state) { const { node } = path; let { scope } = path; + while (scope !== state.jsxScope) { + // If a binding is declared in an inner function, it doesn't affect hoisting. + if (declares(node, scope)) return; + + scope = scope.parent; + } + while (scope) { // We cannot hoist outside of the previous hoisting target // scope, so we return early and we don't update it. @@ -124,72 +141,13 @@ export default declare((api, options) => { state.targetScope = getHoistingScope(scope); }, - - /* - See the discussion at https://github.com/babel/babel/pull/12967#discussion_r587948958 - to uncomment this code. - - 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 && isHoistingScope(scope)) { - 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 (declares(node, scope)) break; - - scope = scope.parent; - } - - if (targetScope) state.targetScope = targetScope; - },*/ }; + // We cannot use traverse.visitors.merge because it doesn't support + // immutabilityVisitor's bare `enter` visitor. + // It's safe to just use ... because the two visitors don't share any key. + const hoistingVisitor = { ...immutabilityVisitor, ...targetScopeVisitor }; + return { name: "transform-react-constant-elements", @@ -216,21 +174,6 @@ export default declare((api, options) => { mutablePropsAllowed = allowMutablePropsOnTags.includes(elementName); } - const state = { - isImmutable: true, - mutablePropsAllowed, - targetScope: path.scope.getProgramParent(), - }; - - // Traverse all props passed to this element for immutability, - // and compute the target hoisting scope - path.traverse(analyzer, state); - - if (!state.isImmutable) return; - - 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 @@ -243,6 +186,18 @@ export default declare((api, options) => { } jsxScope ??= getHoistingScope(path.scope); + const visitorState = { + isImmutable: true, + mutablePropsAllowed, + jsxScope, + targetScope: path.scope.getProgramParent(), + }; + path.traverse(hoistingVisitor, visitorState); + if (!visitorState.isImmutable) return; + + const { targetScope } = visitorState; + HOISTED.set(path.node, targetScope); + // Only hoist if it would give us an advantage. if (targetScope === jsxScope) return; diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist/input.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/basic/input.js similarity index 100% rename from packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist/input.js rename to packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/basic/input.js diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist-member/options.json b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/basic/options.json similarity index 100% rename from packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist-member/options.json rename to packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/basic/options.json diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/basic/output.js similarity index 100% rename from packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist/output.js rename to packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/basic/output.js diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/input.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/input.js new file mode 100644 index 000000000000..13d4482e0d27 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/input.js @@ -0,0 +1,3 @@ +function Component({ increment }) { + return () => value + increment} />; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/options.json b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/options.json new file mode 100644 index 000000000000..af51e6d0eba1 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/options.json @@ -0,0 +1,9 @@ +{ + "plugins": [ + [ + "transform-react-constant-elements", + { "allowMutablePropsOnTags": ["Counter"] } + ], + "syntax-jsx" + ] +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/output.js new file mode 100644 index 000000000000..b567ed3c5702 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/output.js @@ -0,0 +1,7 @@ +function Component({ + increment +}) { + var _Counter; + + return () => _Counter || (_Counter = value + increment} />); +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/input.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/input.js new file mode 100644 index 000000000000..281f5a806f84 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/input.js @@ -0,0 +1,3 @@ +function Component() { + return () => value + prompt("Increment:")} />; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/options.json b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/options.json new file mode 100644 index 000000000000..af51e6d0eba1 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/options.json @@ -0,0 +1,9 @@ +{ + "plugins": [ + [ + "transform-react-constant-elements", + { "allowMutablePropsOnTags": ["Counter"] } + ], + "syntax-jsx" + ] +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/output.js new file mode 100644 index 000000000000..e5a0958d2f9a --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/output.js @@ -0,0 +1,5 @@ +var _Counter; + +function Component() { + return () => _Counter || (_Counter = value + prompt("Increment:")} />); +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/input.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/input.js new file mode 100644 index 000000000000..752e80c14487 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/input.js @@ -0,0 +1,3 @@ +function Component({ value }) { + return () => value + 1} />; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/options.json b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/options.json new file mode 100644 index 000000000000..af51e6d0eba1 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/options.json @@ -0,0 +1,9 @@ +{ + "plugins": [ + [ + "transform-react-constant-elements", + { "allowMutablePropsOnTags": ["Counter"] } + ], + "syntax-jsx" + ] +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/output.js new file mode 100644 index 000000000000..0badd55b3998 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/output.js @@ -0,0 +1,7 @@ +var _Counter; + +function Component({ + value +}) { + return () => _Counter || (_Counter = value + 1} />); +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/input.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/input.js new file mode 100644 index 000000000000..ab58a1a5da1e --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/input.js @@ -0,0 +1,3 @@ +function Component() { + return () => value + 1} />; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/options.json b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/options.json new file mode 100644 index 000000000000..af51e6d0eba1 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/options.json @@ -0,0 +1,9 @@ +{ + "plugins": [ + [ + "transform-react-constant-elements", + { "allowMutablePropsOnTags": ["Counter"] } + ], + "syntax-jsx" + ] +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/output.js new file mode 100644 index 000000000000..d7c6730ab269 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/output.js @@ -0,0 +1,5 @@ +var _Counter; + +function Component() { + return () => _Counter || (_Counter = value + 1} />); +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/input.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/input.js new file mode 100644 index 000000000000..1d7900d1fcfa --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/input.js @@ -0,0 +1,3 @@ +function Component() { + return () => value + prompt("Increment:"))(2)} />; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/options.json b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/options.json new file mode 100644 index 000000000000..af51e6d0eba1 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/options.json @@ -0,0 +1,9 @@ +{ + "plugins": [ + [ + "transform-react-constant-elements", + { "allowMutablePropsOnTags": ["Counter"] } + ], + "syntax-jsx" + ] +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/output.js new file mode 100644 index 000000000000..1d7900d1fcfa --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/output.js @@ -0,0 +1,3 @@ +function Component() { + return () => value + prompt("Increment:"))(2)} />; +} diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist-deopt/input.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/not-allowed-element/input.js similarity index 72% rename from packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist-deopt/input.js rename to packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/not-allowed-element/input.js index e470a4b105e0..e3a1d626526e 100644 --- a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist-deopt/input.js +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/not-allowed-element/input.js @@ -1,5 +1,5 @@ -// This is the same as the pure-expression-whitelist test, but 'FormattedMessage' is *not* whitelisted -// so it should not be hoisted. +// This is the same as the basic test, but 'FormattedMessage' is *not* one of the +// allowed tags so it should not be hoisted. var Foo = React.createClass({ render: function () { return ( diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist-deopt/options.json b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/not-allowed-element/options.json similarity index 100% rename from packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist-deopt/options.json rename to packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/not-allowed-element/options.json diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist-deopt/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/not-allowed-element/output.js similarity index 66% rename from packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist-deopt/output.js rename to packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/not-allowed-element/output.js index b893f6773707..023e4d4b7014 100644 --- a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/pure-expression-whitelist-deopt/output.js +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/not-allowed-element/output.js @@ -1,5 +1,5 @@ -// This is the same as the pure-expression-whitelist test, but 'FormattedMessage' is *not* whitelisted -// so it should not be hoisted. +// This is the same as the basic test, but 'FormattedMessage' is *not* one of the +// allowed tags so it should not be hoisted. var Foo = React.createClass({ render: function () { return