From ba2ba0093c2a81478b64c0212f0c040f78584d9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Sat, 6 Mar 2021 17:43:22 +0100 Subject: [PATCH 1/3] Add tests --- .../basic}/input.js | 0 .../basic}/options.json | 0 .../basic}/output.js | 0 .../allowMutablePropsOnTags/function-closure/input.js | 3 +++ .../function-closure/options.json | 9 +++++++++ .../allowMutablePropsOnTags/function-closure/output.js | 5 +++++ .../function-impure-body/input.js | 3 +++ .../function-impure-body/options.json | 9 +++++++++ .../function-impure-body/output.js | 3 +++ .../function-same-binding-name/input.js | 3 +++ .../function-same-binding-name/options.json | 9 +++++++++ .../function-same-binding-name/output.js | 5 +++++ .../fixtures/allowMutablePropsOnTags/function/input.js | 3 +++ .../allowMutablePropsOnTags/function/options.json | 9 +++++++++ .../fixtures/allowMutablePropsOnTags/function/output.js | 3 +++ .../test/fixtures/allowMutablePropsOnTags/iife/input.js | 3 +++ .../fixtures/allowMutablePropsOnTags/iife/options.json | 9 +++++++++ .../test/fixtures/allowMutablePropsOnTags/iife/output.js | 3 +++ .../not-allowed-element}/input.js | 4 ++-- .../not-allowed-element}/options.json | 0 .../not-allowed-element}/output.js | 4 ++-- .../tag-member-expression}/input.mjs | 0 .../tag-member-expression}/options.json | 0 .../tag-member-expression}/output.mjs | 0 24 files changed, 83 insertions(+), 4 deletions(-) rename packages/babel-plugin-transform-react-constant-elements/test/fixtures/{constant-elements/pure-expression-whitelist => allowMutablePropsOnTags/basic}/input.js (100%) rename packages/babel-plugin-transform-react-constant-elements/test/fixtures/{constant-elements/pure-expression-whitelist-member => allowMutablePropsOnTags/basic}/options.json (100%) rename packages/babel-plugin-transform-react-constant-elements/test/fixtures/{constant-elements/pure-expression-whitelist => allowMutablePropsOnTags/basic}/output.js (100%) create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/input.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/options.json create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/output.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/input.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/options.json create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/output.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/input.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/options.json create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/output.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/input.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/options.json create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/output.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/input.js create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/options.json create mode 100644 packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/iife/output.js rename packages/babel-plugin-transform-react-constant-elements/test/fixtures/{constant-elements/pure-expression-whitelist-deopt => allowMutablePropsOnTags/not-allowed-element}/input.js (72%) rename packages/babel-plugin-transform-react-constant-elements/test/fixtures/{constant-elements/pure-expression-whitelist-deopt => allowMutablePropsOnTags/not-allowed-element}/options.json (100%) rename packages/babel-plugin-transform-react-constant-elements/test/fixtures/{constant-elements/pure-expression-whitelist-deopt => allowMutablePropsOnTags/not-allowed-element}/output.js (66%) rename packages/babel-plugin-transform-react-constant-elements/test/fixtures/{constant-elements/pure-expression-whitelist-member => allowMutablePropsOnTags/tag-member-expression}/input.mjs (100%) rename packages/babel-plugin-transform-react-constant-elements/test/fixtures/{constant-elements/pure-expression-whitelist => allowMutablePropsOnTags/tag-member-expression}/options.json (100%) rename packages/babel-plugin-transform-react-constant-elements/test/fixtures/{constant-elements/pure-expression-whitelist-member => allowMutablePropsOnTags/tag-member-expression}/output.mjs (100%) 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..eb60909dfb36 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/output.js @@ -0,0 +1,5 @@ +function Component({ + increment +}) { + return () => 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..281f5a806f84 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-impure-body/output.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-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..c4b710270a55 --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-same-binding-name/output.js @@ -0,0 +1,5 @@ +function Component({ + value +}) { + return () => 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..ab58a1a5da1e --- /dev/null +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function/output.js @@ -0,0 +1,3 @@ +function Component() { + return () => 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 Date: Sat, 6 Mar 2021 18:02:35 +0100 Subject: [PATCH 2/3] Add support for hoisting functions --- .../src/index.ts | 166 +++++++----------- .../function-closure/output.js | 4 +- .../function-impure-body/output.js | 4 +- .../function-same-binding-name/output.js | 4 +- .../function/output.js | 4 +- 5 files changed, 71 insertions(+), 111 deletions(-) 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..b5d5a862676f 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 immutabiltyVisitor = { 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,67 @@ 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()) 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,70 +138,6 @@ 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; - },*/ }; return { @@ -216,20 +166,13 @@ export default declare((api, options) => { mutablePropsAllowed = allowMutablePropsOnTags.includes(elementName); } - const state = { + const immutabilityState = { 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); + // Traverse all props passed to this element for immutability + path.traverse(immutabiltyVisitor, immutabilityState); + if (!immutabilityState.isImmutable) return; // In order to avoid hoisting unnecessarily, we need to know which is // the scope containing the current JSX element. If a parent of the @@ -243,6 +186,15 @@ export default declare((api, options) => { } jsxScope ??= getHoistingScope(path.scope); + const targetScopeState = { + jsxScope, + targetScope: path.scope.getProgramParent(), + }; + path.traverse(targetScopeVisitor, targetScopeState); + + const { targetScope } = targetScopeState; + 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/allowMutablePropsOnTags/function-closure/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/allowMutablePropsOnTags/function-closure/output.js index eb60909dfb36..b567ed3c5702 100644 --- 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 @@ -1,5 +1,7 @@ function Component({ increment }) { - return () => value + increment} />; + var _Counter; + + return () => _Counter || (_Counter = value + increment} />); } 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 index 281f5a806f84..e5a0958d2f9a 100644 --- 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 @@ -1,3 +1,5 @@ +var _Counter; + function Component() { - return () => value + prompt("Increment:")} />; + return () => _Counter || (_Counter = value + prompt("Increment:")} />); } 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 index c4b710270a55..0badd55b3998 100644 --- 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 @@ -1,5 +1,7 @@ +var _Counter; + function Component({ value }) { - return () => value + 1} />; + return () => _Counter || (_Counter = value + 1} />); } 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 index ab58a1a5da1e..d7c6730ab269 100644 --- 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 @@ -1,3 +1,5 @@ +var _Counter; + function Component() { - return () => value + 1} />; + return () => _Counter || (_Counter = value + 1} />); } From 425f9f712a2be518d5f489ba5ed31fe8af4db9b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=B2=20Ribaudo?= Date: Wed, 16 Feb 2022 01:18:55 +0100 Subject: [PATCH 3/3] Run single traversal --- .../src/index.ts | 29 ++++++++++--------- .../constant-elements/for-loop/output.js | 1 - 2 files changed, 16 insertions(+), 14 deletions(-) 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 b5d5a862676f..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,7 +41,7 @@ export default declare((api, options) => { return scope; } - const immutabiltyVisitor = { + const immutabilityVisitor = { enter(path, state) { const stop = () => { state.isImmutable = false; @@ -81,7 +81,10 @@ export default declare((api, options) => { // If we allow mutable props, tags with function expressions can be // safely hoisted. const { mutablePropsAllowed } = state; - if (mutablePropsAllowed && path.isFunction()) return skip(); + if (mutablePropsAllowed && path.isFunction()) { + path.traverse(targetScopeVisitor, state); + return skip(); + } if (!path.isPure()) return stop(); @@ -140,6 +143,11 @@ export default declare((api, options) => { }, }; + // 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", @@ -166,14 +174,6 @@ export default declare((api, options) => { mutablePropsAllowed = allowMutablePropsOnTags.includes(elementName); } - const immutabilityState = { - isImmutable: true, - mutablePropsAllowed, - }; - // Traverse all props passed to this element for immutability - path.traverse(immutabiltyVisitor, immutabilityState); - if (!immutabilityState.isImmutable) return; - // 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 @@ -186,13 +186,16 @@ export default declare((api, options) => { } jsxScope ??= getHoistingScope(path.scope); - const targetScopeState = { + const visitorState = { + isImmutable: true, + mutablePropsAllowed, jsxScope, targetScope: path.scope.getProgramParent(), }; - path.traverse(targetScopeVisitor, targetScopeState); + path.traverse(hoistingVisitor, visitorState); + if (!visitorState.isImmutable) return; - const { targetScope } = targetScopeState; + const { targetScope } = visitorState; HOISTED.set(path.node, targetScope); // Only hoist if it would give us an advantage. diff --git a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop/output.js b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop/output.js index af1cee1ef5fc..707fcf5a3a5d 100644 --- a/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop/output.js +++ b/packages/babel-plugin-transform-react-constant-elements/test/fixtures/constant-elements/for-loop/output.js @@ -7,4 +7,3 @@ function render() { return nodes; } - \ No newline at end of file