-
Notifications
You must be signed in to change notification settings - Fork 48.5k
[compiler] Correctly insert (Arrow)FunctionExpressions #30446
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
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: 95babb9 Pull Request resolved: #30446
Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: aefc2cd Pull Request resolved: #30446
Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: b432abc Pull Request resolved: #30446
Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: 8a45891 Pull Request resolved: #30446
varDecl.insertAfter( | ||
t.variableDeclaration('const', [ | ||
t.variableDeclarator(compiledFn.id, funcExpr), | ||
]), | ||
); | ||
CompilerError.invariant(typeof varDecl.key === 'number', { | ||
reason: | ||
'Just Babel things: expected the VariableDeclaration containing the compiled function expression to have a number key', | ||
loc: null, | ||
}); | ||
const insertedCompiledFnVarDecl = varDecl.getSibling(varDecl.key + 1); | ||
CompilerError.invariant( | ||
insertedCompiledFnVarDecl.isVariableDeclaration(), | ||
{ | ||
reason: 'Expected inserted sibling to be a VariableDeclaration', | ||
loc: null, | ||
}, | ||
); |
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.
Nit: (please disregard if this is intentional to double check babel-things) I think we might be able to just use the return value of insertAfter
instead of going through getSibling
etc.
``` | ||
|
||
### Eval output | ||
(kind: exception) Fixture not implemented |
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.
Nice! Can we add a fixture entrypoint here?
export const FIXTURE_ENTRYPOINT = {
fn: Component2,
params: [
{
items: [
{id: 2, name: 'foo'},
{id: 3, name: 'bar'},
],
},
],
};
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.
good call, thanks!
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 fix, this makes sense!
I'm a bit confused on the original outlining function-creation logic added by #30331 -- not sure why we want to insert function declarations / expressions / arrows based on the type of the component / hook function. But that's separate of this
Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: cdf36f3 Pull Request resolved: #30446
Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: df13e3b Pull Request resolved: #30446
Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: df13e3b Pull Request resolved: #30446
Previously we would insert new (Arrow)FunctionExpressions as a sibling of the original function. However this would break in the outlining case as it would cause the original function expression's parent to become a SequenceExpression, breaking a bunch of assumptions in the babel plugin. To get around this, we synthesize a new VariableDeclaration to contain the newly inserted function expression and therefore insert it as a true sibling to the original function. Yeah, it's kinda gross ghstack-source-id: df13e3b Pull Request resolved: facebook#30446
Thanks for the fix. I think we can simplify this by always creating the outlined function as a function declaration at the top level of the program. The name is already guaranteed to be unique in the file so this won’t cause conflicts. |
I reused the existing logic to create new functions and it didn’t seem to really matter in practice what type of function we created. But yeah we should change this to always produce a FunctionDeclaration for outlined functions. |
Addresses follow up feedback from #30446. Since the outlined function is guaranteed to have a module-scoped unique identifier name, we can simplify the insertion logic for the outlined function to always emit a function declaration rather than switch based on the original function type. This is fine because the outlined function is synthetic anyway. ghstack-source-id: 0a4d1f7 Pull Request resolved: #30464
Addresses follow up feedback from #30446. Since the outlined function is guaranteed to have a module-scoped unique identifier name, we can simplify the insertion logic for the outlined function to always emit a function declaration rather than switch based on the original function type. This is fine because the outlined function is synthetic anyway. ghstack-source-id: 0a4d1f7 Pull Request resolved: #30464
Stack from ghstack (oldest at bottom):
Previously we would insert new (Arrow)FunctionExpressions as a sibling
of the original function. However this would break in the outlining case
as it would cause the original function expression's parent to become a
SequenceExpression, breaking a bunch of assumptions in the babel plugin.
To get around this, we synthesize a new VariableDeclaration to contain
the newly inserted function expression and therefore insert it as a true
sibling to the original function.
Yeah, it's kinda gross