-
Notifications
You must be signed in to change notification settings - Fork 50k
[compiler] Add enableUseKeyedState flag and update setState validation #35230
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
This commit adds a new compiler environment flag `enableUseKeyedState`, and updates ValidateNoSetStateInRender to no longer throw on setState calls with primitive values. When `enableUseKeyedState` is enabled, calling setState with a primitive instead recommends using `useKeyedState` instead.
| ); | ||
| let isArgPrimitive = false; | ||
|
|
||
| if (instr.value.args.length > 0) { |
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 think we should consider a setState with no arguments to be a primitive right? Because we would be changing the state to undefined?
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 know we discussed the idea of allowing unconditional setState-in-render calls if you pass a primitive, but then we discussed that we could recommend useKeyedState() (behind a flag) and that feels like it is the more robust option. We had a question mark on whether we should allow unconditional calls w primitives in our diagram.
As a slight variation, we could continue to flag all cases of setState in render with an error along the lines of:
Error: Cannot call setState during render
Calling setState during render may trigger an infinite loop.
* To reset one state when other state/props change, use `const [state, setState] = useKeyedState(initialState, key)` to reset `state` when `key` changes.
* To derive data from other state/props, compute the derived data during render without using state.
Where the first bullet would change conditional upon enableUseKeyedState (the abvoe example is assuming it's enabled, if false suggest the manual implementation of useKeyedState)
| if (instr.value.args.length > 0) { | ||
| const arg = instr.value.args[0]; | ||
| if (arg.kind === 'Identifier') { | ||
| isArgPrimitive = isPrimitiveSetArg(arg, fn); |
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 expected that this would be something like: if (!arg.reactive && HIR.isPrimitiveType(arg.identifier)) { /* allow */ } - ie reusing type inference to determine if a value is a primitive. And likely combining with a !arg.reactive check because if the value being set is reactive, then it could still infinite loop even if its a primitive.
But see the larger comment about the overall direction and whether we want to allow passing primitives
This commit adds a new compiler environment flag
enableUseKeyedState, andupdates ValidateNoSetStateInRender to no longer throw on setState calls with primitive values. When
enableUseKeyedStateis enabled, calling setState with a primitive instead recommends usinguseKeyedStateinstead.