-
Notifications
You must be signed in to change notification settings - Fork 48.8k
fix(react-compiler): optimize components declared with arrow function and implicit return and compilationMode: 'infer'
#31792
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-compiler): optimize components declared with arrow function and implicit return and compilationMode: 'infer'
#31792
Conversation
…s and implicit returns with `compilationMode: 'infer'`
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
compilationMode: 'infer'
compilationMode: 'infer'
compilationMode: 'infer'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! Please see comments
if (node.type === 'ArrowFunctionExpression' && node.node.body.type === 'JSXElement') { | ||
return false | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this PR the first time around, my apologies. This is an overly narrow solution and needs to be more robust. Let's do the following:
- Extract out a helper that takes an Expression and returns a boolean for whether its possibly a React node or not. This can reuse the
switch (argument.type) { ... }
from the current ReturnStatement case. - Reuse this helper in the existing ReturnStatement case
- Expand the ArrowFunctionExpression case to check if the body is an expresssion, and if so use the new helper. otherwise traverse with skipNestedFunctions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No worries Joseph, thanks for your detailed feedback 🤩
Extract out a helper that takes an Expression and returns a boolean for whether its possibly a React node or not. This can reuse the switch (argument.type) { ... } from the current ReturnStatement case.
extracted as isNonNode
function in d0f5b19
react/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
Lines 996 to 1010 in d0f5b19
function isNonNode(node?: t.Expression | null): boolean { | |
if (!node) { | |
return true; | |
} | |
switch (node.type) { | |
case 'ObjectExpression': | |
case 'ArrowFunctionExpression': | |
case 'FunctionExpression': | |
case 'BigIntLiteral': | |
case 'ClassExpression': | |
case 'NewExpression': // technically `new Array()` is legit, but unlikely | |
return true; | |
} | |
return false; | |
} |
Reuse this helper in the existing ReturnStatement case
react/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts
Lines 1029 to 1032 in d0f5b19
ReturnStatement(ret) { | |
hasReturn = true; | |
returnsNonNode = isNonNode(ret.node.argument); | |
}, |
Expand the ArrowFunctionExpression case to check if the body is an expresssion, and if so use the new helper. otherwise traverse with skipNestedFunctions.
I can't get this to work, seems node.traverse#ArrowFunctionExpression
isn't called for the root node
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also removed useless hasReturn
variable in e0c9364
@josephsavona friendly ping |
Thanks for the fix! |
fixes #31601 #31639 cc @josephsavona