Lazily initialize and cache constant JSX elements

[nicolo-ribaudo]

Q	A
Fixed Issues?	Fixes #11686, fixes #12422, fixes #12237, fixes #9965, fixes #12303, fixes #10879, fixes #7610, fixes #7565, fixes #7726, fixes #6751. I think this also fixes #10878 because with this PR the plugin doesn't move JSX elements around, but there isn't an example to confirm it.
Patch: Bug Fix?	Yes
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

This PR rewrites the @babel/plugin-transform-constant-elements implementation under a new realization: we don't need to actually hoist the elements to prevent them from being unnecessarily evaluated multiple times, we just need to cache them.

This also has the potential effect of improving app startup performance, by only computing JSX elements when they are needed rather than when the app is first loaded.

After this PR, this code (260 lines) becomes unused in this repository. We could try removing it in Babel 8 if no important community plugins depend on it (it has different bugs).

UPDATE: I just realized my idea isn't new: facebook/react#3226 (comment)

cc @babel/react

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/44001/

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit a4e8550:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[jridgewell]

Before I forget while reviewing: #12422 (comment) made me think of another case:

import { Foo } from "./main.js";

export function bar() {
  return <Foo />;
}

This element is not provably constant, because Foo is a live binding and can be changed inside main.js.

[nicolo-ribaudo]

That's technically true, but:

• this plugin has always operated under the assumption that it's constant, and no one ever complained
• I have never seen a react codebase in my life where assuming that it's constant could cause problems
• it would completely disable the optimization for all the non-builtin elements (since almost always they are imported)

I would prefer to leave it as-is, and possibly change it in the future if needed (with a flag to opt-in or opt-out).

[nicolo-ribaudo]

I'm not sure I like caching this. For now let's keep the current behavior since no one complained, but this doesn't look safe (I don't know how React components on this are usually used).

[jridgewell]

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;
 

[nicolo-ribaudo]

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 needsHoisting variable.

[jridgewell]

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.

[nicolo-ribaudo]

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>);
}

[jridgewell]

I think we'll be able to handle that case with the simplified code, but let's wait for the followup PR.

[jridgewell]

I would prefer to leave it as-is, and possibly change it in the future if needed (with a flag to opt-in or opt-out).

Going through the examples, it seems we treat member expressions as always constant, eg for Intl.FormattedMessage. Given that, I think this is fine.

[nicolo-ribaudo]

I realized that we don't hoist/cache elements with functions:

function Counter({ inc }) {
  let [value, update] = useState(0);

  return <button onClick={() => update(inc)}>{value}</button>;
}

function Main() {
  return <Counter inc={num => num + 2} />;
}

could become

var _Counter;

function Counter({ inc }) {
  let [value, update] = useState(0);

  return <button onClick={() => update(inc)}>{value}</button>;
}

function Main() {
  return _Counter || (_Counter = <Counter inc={num => num + 2} />);
}

this PR already contains the logic to track bindings in nested functions, but I'd prefer to actually allow hoisting of components with functions in a separate PR to keep this 1:1 with the previous behavior.

[nicolo-ribaudo]

In Babel 8 we could just use ||= here, and if someone targets an older browser it will be transformed by another different plugin.

[JLHwung]

Looks good to me other than that we may miss the namespaced name, but we didn't support it before this PR anyway.

[JLHwung]

name can also be a JSXNamespacedName, which is currently not handled.

Can you add a test case like

function AComponent () {
  return <BComponent/>

  function BComponent () {
    return <n:CComponent />
  }
}

n:CComponent should be cached.

[jridgewell]

Suggested change

	if (path.parentPath.isJSXElement()) {
	if (path.parentPath.isJSXElement() || path.parentPath.isJSXAttribute()) {

Test Case:

function C(x) {
  // Bind the div in this scope, so only span is hoisted.
  return <div x={x} attr=<span /> />;
}

[jridgewell]

Suggested change

	while (!jsxScope && current.parentPath.isJSXElement()) {
	while (!jsxScope && current.parentPath.isJSX()) {

Test Case:

function C() {
  return <div attr=<span /> />;
}

[jridgewell]

I think we'll be able to handle that case with the simplified code, but let's wait for the followup PR.

[existentialism]

💯