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

[compiler] Support outlined fns for auto-deps #31662

Closed
wants to merge 0 commits into from
Closed

[compiler] Support outlined fns for auto-deps #31662

wants to merge 0 commits into from
+0 −0

Conversation

jbrown215
Copy link
Contributor

@jbrown215 jbrown215 commented Dec 3, 2024 •
edited
Loading

Summary:

We can keep track of outlined functions and insert an empty dep array when we find one passed to an effect. Outlined functions
always have no deps!

Outlined functions are given unique names, so we can look at LoadGlobals and see if one of those names is used.

Stack created with Sapling. Best reviewed with ReviewStack.

@vercel Vercel
Copy link

vercel bot commented Dec 3, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Dec 3, 2024 5:17pm

This was referenced Dec 3, 2024
Closed
Closed
mofeiZ
mofeiZ approved these changes Dec 3, 2024
View reviewed changes
Copy link
Contributor

@mofeiZ mofeiZ left a comment

Choose a reason for hiding this comment

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

shipit!

mofeiZ
mofeiZ reviewed Dec 3, 2024
View reviewed changes
...ages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/outlined-function.expect.md Outdated
@@ -34,7 +34,7 @@ import { print } from "shared-runtime";
* before OutlineFunctions
*/
function OutlinedFunctionInEffect() {
useEffect(_temp);
useEffect(_temp, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

nice!

@josephsavona
Copy link
Contributor

I don't get it - shouldn't outlined functions already be a LoadGlobal, which doesn't need to be a dependency?

@jbrown215
Copy link
Contributor Author

I don't get it - shouldn't outlined functions already be a LoadGlobal, which doesn't need to be a dependency?

hmm, so IIUC we don't need to keep track of outlined functions because it's sufficient to check if the argument is a LoadGlobal? Sounds reasonable to me

@jbrown215 jbrown215 closed this Dec 3, 2024
@jbrown215 jbrown215 deleted the pr31662 branch December 3, 2024 17:14
@jbrown215 jbrown215 mentioned this pull request Dec 3, 2024
Merged
@josephsavona
Copy link
Contributor

Yeah exactly

jbrown215 added a commit that referenced this pull request Dec 3, 2024
@jbrown215
[compiler] Empty dep arrays for globals/module-scoped values/imports (#…
6bcf0d2 
…31666)

Any LoadGlobal in the "infer deps" position can safely use an empty dep
array. Globals have no reactive deps!

I just keep messing up sapling. This is the revised version of #31662
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mofeiZ mofeiZ mofeiZ approved these changes

Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jbrown215 @josephsavona @mofeiZ @facebook-github-bot