[compiler] Don't include current field accesses in auto-deps #31652
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| /* | ||
| * Prune ref.current accesses. This may over-capture for non-ref values with | ||
| * a current property, but that's fine. | ||
| */ | ||
| currValue.effect = Effect.Freeze; | ||
| break; |
There was a problem hiding this comment.
i haven't followed the implementation of auto-deps, but why are we changing the effects here? if we're pruning i'd expect that to be operating on the dependency path but not changing effects, which should have already been inferred.
There was a problem hiding this comment.
This is just unnecessary and I have no idea how that line ended up here
you're fast on draft diffs :P
There was a problem hiding this comment.
@josephsavona The freeze here is redundant with a later freeze (before the function return). The reason we're manually setting a Effect.Freeze is because this pass inserts new instructions with new Places.
We could set the effect to be Effect.ConditionallyMutate, but inserted auto-deps really has freeze effects. This really should be a no-op with enableTransitivelyFreezeFunctionExpressions, which already freezes captured dependencies. One thing we could do is validate enableTransitivelyFreezeFunctionExpressions is set with inferEffectDependencies.
jbrown215
changed the title
jbrown215
changed the title
jbrown215
changed the title
Unfortunately I saw multiple cases in my first experiment where the This is the more conservative approach for sure. We can use types to get more precise and error when people use React.RefObjects without a ref suffix in a component/hook |
jbrown215
changed the title
jbrown215
changed the title
jbrown215
changed the title
jbrown215
changed the title
## Summary Drops .current field accesses in inferred dep arrays
## Summary Allows us to add deps for things like `import useWrapperEffect from 'useWrapperEffect'` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31657). * __->__ #31657 * #31652
Summary
Drops .current field accesses in inferred dep arrays
Stack created with Sapling. Best reviewed with ReviewStack.