[autodeps] Do not include nonreactive refs or setStates in inferred deps#32236
Conversation
github-actions
Bot
added
the
React Core Team
| // If it is reactive OR it is not reactive and neither a ref or setState call, then we need to add it to the deps array | ||
| if ( | ||
| reactiveIds.has(dep.identifier.id) || | ||
| (!isUseRefType(dep.identifier) && !isSetStateType(dep.identifier)) |
There was a problem hiding this comment.
Reactivity inference should already account for these types, why do we need to check them here? Note that you could still have a ref or state setter that is conditional on some other reactive value.
There was a problem hiding this comment.
@josephsavona We can't prune purely based on reactivity inference (see above comment and infer-effect-deps/pruned-nonreactive-obj
I think we have 4 cases here. On one axis, there's reactivity (per data-flow analysis pre scope creation and merging/pruning)
| Reactive | Non reactive | |
|---|---|---|
| hook return value | dep ✅ | not dep 🚫 |
| other value | dep ✅ | might be dep if included into a reactive scope❓ |
@jbrown215 do you think it's easier to reason about this when excluding the (isUseRef || isSetState) && !reactive case, e.g.
for (const dep of scopeInfo.deps) {
if ((isUseRef(dep.identifier) || isSetState(dep.identifier) && !reactiveIds.has(dep.identifier.id) {
// exclude non-reactive hook results, which will never be in a memo block
continue;
}
...
}There was a problem hiding this comment.
That version of it looks good to me! Getting this condition right was pretty tricky.
| } else { | ||
| t0 = $[3]; | ||
| } | ||
| useEffect(t0, [ref]); |
Summary: Summary: Correctly supports React.useEffect when React is imported as `import * as React from 'react'` (as well as other namespaces as specified in the config). Test Plan:
4 participants
Stack created with Sapling. Best reviewed with ReviewStack.