-
Notifications
You must be signed in to change notification settings - Fork 47.4k
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
[compiler][rfc] Hacky retry pipeline for fire #32164
base: main
Are you sure you want to change the base?
Conversation
Start of a simple retry pipeline. Would love feedback on how we implement this to be extensible to other compiler non-memoization features (e.g. inlineJSX)
8 | useEffect(() => { | ||
> 9 | fire(foo(props), bar); | ||
| ^^^^^^^^^^^^^^^^^^^^^ InvalidReact: Cannot compile `fire`. fire() can only take in a single call expression as an argument but received multiple arguments (9:9) | ||
|
||
InvalidReact: Cannot compile `fire`. fire() can only take in a single call expression as an argument but received a spread argument (10:10) | ||
|
||
InvalidReact: Cannot compile `fire`. `fire()` can only receive a function call such as `fire(fn(a,b)). Method calls and other expressions are not allowed (11:11) | ||
|
||
InvalidReact: Cannot compile `fire`. `fire()` can only receive a function call such as `fire(fn(a,b)). Method calls and other expressions are not allowed (12:12) | ||
10 | fire(...foo); | ||
11 | fire(bar); | ||
12 | fire(props.foo()); | ||
``` |
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.
Note that we continue to emit fire-related bailouts
// @enableFire | ||
import { fire } from "react"; | ||
|
||
function Component({ props, bar }) { | ||
"use no memo"; | ||
const foo = () => { | ||
console.log(props); | ||
}; | ||
useEffect(() => { | ||
fire(foo(props)); | ||
fire(foo()); | ||
fire(bar()); | ||
}); | ||
|
||
return 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.
This is a todo fixture because "use no memo"
directives probably should directly invoke the retry pipeline (e.g. no memo, no validation, just lower -> transformFire -> codegen)
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.
make sense
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.
awesome!
Re the "hacky" framing of this in the PR title: I think this is fine. I can see having something like a compilation or output mode that we use when running the pipeline. Rather than changing environment settings when we re-run, various passes could take effect based on the combination of the mode + env flags. The modes might be:
|
Hacky retry pipeline for when transforming
fire(...)
calls encounters validation, todo, or memoization invariant bailouts. Would love feedback on how we implement this to be extensible to other compiler non-memoization features (e.g. inlineJSX)Some observations:
Note the newly added fixtures are prefixed with
bailout-...
when the retry fire pipeline is used. These fixture outputs contain correctly inserteduseFire
calls and no memoization.