Fix React Strictmode for sliders #4012
Conversation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| const isDraggingsRef = useRef<boolean[]>(null); | ||
| isDraggingsRef.current = isDraggings; | ||
| useEffect(() => { |
There was a problem hiding this comment.
can't change this to a layouteffect since it's in stately and we can't use our SSR safe version there since it's in aria's utils
There was a problem hiding this comment.
There was a problem hiding this comment.
It turned out that we don't even need to assign these refs during render. We only need them so that we can fire onChangeEnd immediately after onChange without waiting for a render in between. If we always assign them at the same time we update the state it seems to work. Done in #4564.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| if (isControlled) { | ||
| stateRef.current = value; | ||
| } | ||
| }); |
There was a problem hiding this comment.
I still think this is too late. Imagine we have a controlled component. We fire onChange, this triggers a re-render of the parent component. Then another onChange fires before the useEffect is called, but we would be comparing against the previous value in stateRef.current. This is definitely a problem with useEffect because it runs after an animation frame, but I think it's even a problem with useLayoutEffect.
For example, say you have two components: a parent and a child. The parent component has a useControlledState in it, and passes its onChange function to the child. The child has a useLayoutEffect in it which triggers an onChange in response to something. This is fairly common, for example combobox reacts to prop changes in an effect and might emit events based on that. Same with auto selecting the first item in tabs on mount. The problem is that child effects run before parent effects. If the child component calls onChange in its effect, the value in the ref within the parent component won't have updated yet, meaning the comparison we do would not be correct. You can see an example of that here - the stateRef is always behind by one render.
Honestly, I'm not really sure how to solve this. For React 18, we might be able to use useInsertionEffect which runs before layout effects. But you'd still have the same problem if any other component used useInsertionEffect (rare), and for older Reacts.
Updating during render is actually the most close to correct. It's only incorrect when a render gets aborted (e.g. during suspense). For strict mode, the value coming from props should be the same between the two renders so it shouldn't matter. Did you see test failures due to this, or was this just following the linter?
There was a problem hiding this comment.
Went back and checked the tests passing before and after. It's the same. So changing useControlledState was just following the linter.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
snowystinger
changed the title
|
Build successful! 🎉 |
|
## API Changes
unknown top level export { type: 'identifier', name: 'Column' } |
Fixes sliders and color components (alternative to #4012).
| state.setThumbDragging(realTimeTrackDraggingIndex.current, false); | ||
| realTimeTrackDraggingIndex.current = null; | ||
| } | ||
| }); |
There was a problem hiding this comment.
I think these useEffectEvents should really be inside useMove so that other consumers benefit as well. I made this change in #4564.
| stateRef.current = state; | ||
| useLayoutEffect(() => { | ||
| stateRef.current = state; | ||
| }); |
There was a problem hiding this comment.
We can get rid of stateRef completely if we make useMove use useEffectEvent. Done in #4564.
| const isDraggingsRef = useRef<boolean[]>(null); | ||
| isDraggingsRef.current = isDraggings; | ||
| useEffect(() => { |
There was a problem hiding this comment.
It turned out that we don't even need to assign these refs during render. We only need them so that we can fire onChangeEnd immediately after onChange without waiting for a render in between. If we always assign them at the same time we update the state it seems to work. Done in #4564.
|
Closing in favor of #4564 |
Closes
Note on tests, failure count may be inaccurate due to cascading failures
Before
Test Suites: 27 failed, 1 skipped, 144 passed, 171 of 172 total
Tests: 657 failed, 49 skipped, 3534 passed, 4240 total
Snapshots: 1 passed, 1 total
After
Test Suites: 24 failed, 1 skipped, 147 passed, 171 of 172 total
Tests: 654 failed, 49 skipped, 3537 passed, 4240 total
Snapshots: 1 passed, 1 total
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: