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

fix: react-cons-elem should not hoist router comp #14828

Merged
merged 1 commit into from Aug 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
@@ -1,6 +1,6 @@
import { declare } from "@babel/helper-plugin-utils";
import { types as t, template } from "@babel/core";
import type { Visitor, Scope } from "@babel/traverse";
import type { Visitor, Scope, NodePath } from "@babel/traverse";

export interface Options {
allowMutablePropsOnTags?: null | string[];
Expand Down Expand Up @@ -165,8 +165,6 @@ export default declare((api, options: Options) => {
visitor: {
JSXElement(path) {
if (HOISTED.has(path.node)) return;
HOISTED.set(path.node, path.scope);

const name = path.node.openingElement.name;

// This transform takes the option `allowMutablePropsOnTags`, which is an array
Expand All @@ -191,13 +189,15 @@ export default declare((api, options: Options) => {
// 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;
let current: NodePath<t.JSX> = path;
while (!jsxScope && current.parentPath.isJSX()) {
// @ts-expect-error current is a search pointer
current = current.parentPath;
jsxScope = HOISTED.get(current.node);
}
jsxScope ??= path.scope;
// The initial HOISTED is set to jsxScope, s.t.
// if the element's JSX ancestor has been hoisted, it will be skipped
HOISTED.set(path.node, jsxScope);

const visitorState: VisitorState = {
isImmutable: true,
Expand All @@ -209,8 +209,6 @@ export default declare((api, options: Options) => {
if (!visitorState.isImmutable) return;

const { targetScope } = visitorState;
HOISTED.set(path.node, targetScope);

// Only hoist if it would give us an advantage.
for (let currentScope = jsxScope; ; ) {
if (targetScope === currentScope) return;
Expand All @@ -228,6 +226,8 @@ export default declare((api, options: Options) => {

const id = path.scope.generateUidBasedOnNode(name);
targetScope.push({ id: t.identifier(id) });
// If the element is to be hoisted, update HOISTED to be the target scope
HOISTED.set(path.node, targetScope);

let replacement: t.Expression | t.JSXExpressionContainer = template
.expression.ast`
Expand Down
@@ -0,0 +1,21 @@
import { Routes, Route } from "react-router";
import { router } from "common/router";

function RoutesComponent() {
return <Routes>
{Object.keys(router).map(routerKey => {
const route = router[routerKey];

if (route && route.element) {
const {
path,
element: Component
} = route;
// Component should not be hoisted
return <Route key={routerKey} path={path} element={<Component />} />;
} else {
return null;
}
}).filter(Boolean)}
</Routes>;
}
@@ -0,0 +1,21 @@
import { Routes, Route } from "react-router";
import { router } from "common/router";

function RoutesComponent() {
return <Routes>
{Object.keys(router).map(routerKey => {
const route = router[routerKey];

if (route && route.element) {
const {
path,
element: Component
} = route; // Component should not be hoisted

return <Route key={routerKey} path={path} element={<Component />} />;
} else {
return null;
}
}).filter(Boolean)}
</Routes>;
}