Skip to content
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

Merged
merged 12 commits into from Mar 6, 2021

Conversation

@nicolo-ribaudo
Copy link
Member

@nicolo-ribaudo nicolo-ribaudo commented Mar 4, 2021

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
Copy link
Collaborator

@babel-bot babel-bot commented Mar 4, 2021

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

@codesandbox
Copy link

@codesandbox codesandbox bot commented Mar 4, 2021

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
Copy link
Member

@jridgewell jridgewell commented Mar 4, 2021

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
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Mar 4, 2021

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 and others added 2 commits Mar 5, 2021
…dex.js

Co-authored-by: Justin Ridgewell <justin@ridgewell.name>

this.render = () => _ref2;
this.render = () => _this$subComponent || (_this$subComponent = <this.subComponent />);

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Mar 5, 2021
Author Member

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).

@nicolo-ribaudo nicolo-ribaudo changed the title Fix all the transform-react-constant-elements bugs Lazily initialize and cache constant JSX elements Mar 5, 2021
let targetScope;

let isNestedScope = true;
let needsHoisting = true;

This comment has been minimized.

@jridgewell

jridgewell Mar 5, 2021
Member

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;
 

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Mar 5, 2021
Author Member

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.

This comment has been minimized.

@jridgewell

jridgewell Mar 5, 2021
Member

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.

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Mar 5, 2021
Author Member

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

This comment has been minimized.

@jridgewell

jridgewell Mar 5, 2021
Member

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

Copy link
Member

@jridgewell jridgewell left a comment

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 and others added 2 commits Mar 5, 2021
…dex.js

Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
@nicolo-ribaudo
Copy link
Member Author

@nicolo-ribaudo nicolo-ribaudo commented Mar 5, 2021

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.

function _action() {
_action = babelHelpers.asyncToGenerator(function* () {
return _ref;
return _Contact || (_Contact = /*#__PURE__*/React.createElement(Contact, {

This comment has been minimized.

@nicolo-ribaudo

nicolo-ribaudo Mar 5, 2021
Author Member

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

@JLHwung JLHwung self-requested a review Mar 5, 2021
Co-authored-by: Justin Ridgewell <justin@ridgewell.name>
@JLHwung
JLHwung approved these changes Mar 5, 2021
Copy link
Contributor

@JLHwung JLHwung left a comment

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

let namePath = path.get("openingElement.name");
while (namePath.isJSXMemberExpression()) {
namePath = namePath.get("property");
let lastSegment = name;

This comment has been minimized.

@JLHwung

JLHwung Mar 5, 2021
Contributor

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.

let replacement = template.expression.ast`
${t.identifier(id)} || (${t.identifier(id)} = ${path.node})
`;
if (path.parentPath.isJSXElement()) {

This comment has been minimized.

@jridgewell

jridgewell Mar 5, 2021
Member

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 /> />;
}
// scope as the base scope for the current element.
let jsxScope;
let current = path;
while (!jsxScope && current.parentPath.isJSXElement()) {

This comment has been minimized.

@jridgewell

jridgewell Mar 5, 2021
Member

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

Test Case:

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

let isNestedScope = true;
let needsHoisting = true;

This comment has been minimized.

@jridgewell

jridgewell Mar 5, 2021
Member

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

Copy link
Member

@existentialism existentialism left a comment

💯

This was referenced Mar 17, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.