-
Notifications
You must be signed in to change notification settings - Fork 49.9k
[compiler] Extend setState in effect validation to useEffectEvent #35214
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
base: main
Are you sure you want to change the base?
Conversation
ValidateNoSetStateInEffects already supports transitive setter functions. This PR marks any synchonous state setter useEffectEvent function so we can validate that uEE isn't being used only as misdirection to avoid the validation within an effect body. The error points to the call of the effect event.
| ? instr.value.receiver | ||
| : instr.value.callee; | ||
|
|
||
| if (isUseEffectEventType(callee.identifier)) { |
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 would only catch useEffectEvent function type calls at the top level in a useEffect right? Should we consider something like this?
const [state, setState] = useState(0)
const effectEvent = useEffectEvent(() => {
setState(true)
});
useEffect(() => {
function foo() {
effectEvent()
}
someLogic();
foo();
}, [])
I'm just curious. Would this code even work? I'm not very familiar with uEE
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.
I don't think there's anything to specific about uEE in that example: it's just a question of whether we check recursive function expression (looks like maybe not, would be an easy extension). But that's an orthogonal change.
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.
I double-checked, we don't currently catch this case if you swap the effectEvent() call for a direct setState() call. Definitely worth adding — great catch, @jorge-cab! — but not strictly related to this PR
| setStateFunctions.set(instr.lvalue.identifier.id, setState); | ||
| } | ||
| } | ||
| } |
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.
can do else if to connect w the existing condition, since they can't both be true
|
I actually bumped into this one today and I'm not sure what the intended solution for "did mount" flag is. Specifically, if you need to use some browser-only stuff in JSX, so you're forced to separate hydration in two stages (initial server content -> client content). See https://react.dev/reference/react/useEffect#displaying-different-content-on-the-server-and-the-client for this use case described in the docs. |
ValidateNoSetStateInEffects already supports transitive setter functions. This PR marks any synchonous state setter useEffectEvent function so we can validate that uEE isn't being used only as misdirection to avoid the validation within an effect body.
The error points to the call of the effect event.
Example: