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.

Sign up for GitHub

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

Jump to bottom

Never hoist JSX elts referencing vars from the current scope #14536

Merged
merged 1 commit into from May 9, 2022
Merged

Never hoist JSX elts referencing vars from the current scope #14536

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 14 additions & 2 deletions packages/babel-plugin-transform-react-constant-elements/src/index.ts
View file
Open in desktop
Expand Up @@ -197,7 +197,7 @@ export default declare((api, options: Options) => {
current = current.parentPath;
jsxScope = HOISTED.get(current.node);
}
jsxScope ??= getHoistingScope(path.scope);
jsxScope ??= path.scope;

const visitorState: VisitorState = {
isImmutable: true,
Expand All @@ -212,7 +212,19 @@ export default declare((api, options: Options) => {
HOISTED.set(path.node, targetScope);

// Only hoist if it would give us an advantage.
if (targetScope === jsxScope) return;
for (let currentScope = jsxScope; ; ) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now more complex, because in cases like

function f() {
  let Component;
  {
    return <Component />;
  }
}

jsxScope is the block statement while targetScope is the function: we still do not want to hoist the element, because it's unnecessary (there are already 2-3 tests covering this).

if (targetScope === currentScope) return;
if (isHoistingScope(currentScope)) break;

currentScope = currentScope.parent;
if (!currentScope) {
throw new Error(
"Internal @babel/plugin-transform-react-constant-elements error: " +
"targetScope must be an ancestor of jsxScope. " +
"This is a Babel bug, please report it.",
);
}
}

const id = path.scope.generateUidBasedOnNode(name);
targetScope.push({ id: t.identifier(id) });
Expand Down
12 changes: 12 additions & 0 deletions ...ansform-react-constant-elements/test/fixtures/constant-elements/regression-14363/input.js
View file
Open in desktop
@@ -0,0 +1,12 @@
function RoutesComponent() {
return (
<Routes>
{(c) => {
{
const Component = c;
return <Component />;
}
}}
</Routes>
);
}
10 changes: 10 additions & 0 deletions ...nsform-react-constant-elements/test/fixtures/constant-elements/regression-14363/output.js
View file
Open in desktop
@@ -0,0 +1,10 @@
function RoutesComponent() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old output was

var _Component;

function RoutesComponent() {
  return (
    <Routes>
      {(c) => {
        {
          const Component = c;
          return _Component || (_Component = <Component />);
        }
      }}
    </Routes>
  );
}

return <Routes>
{c => {
{
const Component = c;
return <Component />;
}
}}
</Routes>;
}