[compiler] Empty dep arrays for globals/module-scoped values/imports #31666
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| const numRequiredArgs = moduleTargets.get(DEFAULT_EXPORT); | ||
| if (numRequiredArgs != null) { | ||
| autodepFnLoads.set(lvalue.identifier.id, numRequiredArgs); | ||
| } else if (value.kind === 'LoadGlobal') { |
There was a problem hiding this comment.
I consolidated all of the loadglobal cases here and merged the autodepFnConfig lookups
There was a problem hiding this comment.
Nice! Could we also update the PR title and description (e.g. no deps array for all global / module / imports, not just outlined fns)
| @@ -117,8 +116,19 @@ export function inferEffectDependencies(fn: HIRFunction): void { | |||
| autodepFnLoads.get(value.callee.identifier.id) === value.args.length && | |||
| value.args[0].kind === 'Identifier' | |||
There was a problem hiding this comment.
Could we check value.args.length or moduleTargets in a followup? Either bailing out or skipping seems reasonable, but this looks like it might crash compilation on useInferEffect() calls with no args.
(I might be wrong though, could have missed a Environment validation fn!)
There was a problem hiding this comment.
I can add an environment validation for it. That number should always be greater than 0, otherwise there is no lambda to analyze
| @@ -177,6 +177,16 @@ export function inferEffectDependencies(fn: HIRFunction): void { | |||
| // Step 2: push the inferred deps array as an argument of the useEffect | |||
| value.args.push({...depsPlace, effect: Effect.Freeze}); | |||
| rewriteInstrs.set(instr.id, newInstructions); | |||
| } else if (loadGlobals.has(value.args[0].identifier.id)) { | |||
| // We outlined the function expression, so we know it has no dependencies | |||
| @@ -34,7 +34,7 @@ import { print } from "shared-runtime"; | |||
| * before OutlineFunctions | |||
| */ | |||
| function OutlinedFunctionInEffect() { | |||
| useEffect(_temp); | |||
| useEffect(_temp, []); | |||
There was a problem hiding this comment.
Looks good! Please see the value.args.length / validating Environment.inferEffectDependencies comment though.
repro in snap:
- change
numRequiredArgsto 0 in Environment test defaults
// @inferEffectDependencies
import {useEffect} from 'react';
function Foo({arg}) {
useEffect();
}
See babel pipeline failure
FAIL: todo
>> Unexpected error during test:
Expected fixture `todo` to succeed but it failed with error:
/todo.ts: Cannot read properties of undefined (reading 'kind')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.
jbrown215
changed the title
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