Fix remaining React strict mode issues #4564
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ReusableView can be mutated, so we need to pass the layoutInfo/content at the time we render to the JSX element rather than reading directly 122
We used to focus the collection itself when the focused item went out of view, but we don't need this anymore because we use persistedKeys to ensure the focused item is always in the DOM. In strict mode this was causing some weird behavior due to ordering of the useEffects. We also need to ensure that when a DOM node is reused for a different item that we do actually move focus.
Escape should take you back to the original drag target, and should not affect the focusedKey of the target list.
Fixes sliders and color components (alternative to #4012).
Incorporates #4035
Doing this in render was actually wrong anyway because if you had the tabs in a controlled state, it would fire onSelectionChange during render which would result in react erroring about updating a different component while another was rendering. Doing it in useEffect is the right time.
From #4005
|
Build successful! 🎉 |
snowystinger
previously approved these changes
May 31, 2023
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
## API Changes
unknown top level export { type: 'identifier', name: 'Column' } @react-aria/utilsuseEffectEvent-
+useDeepMemo<T> {
+ value: T
+ isEqual: (T, T) => boolean
+ returnVal: undefined
+}@react-aria/virtualizerVirtualizer Virtualizer<T extends {}, V> {
+ autoFocus?: boolean
children: (string, {}) => V
collection: Collection<{}>
focusedKey?: Key
isLoading?: boolean
onLoadMore?: () => void
renderWrapper?: (ReusableView<{}, V> | null, ReusableView<{}, V>, Array<ReusableView<{}, V>>, (Array<ReusableView<{}, V>>) => Array<ReactElement>) => ReactElement
scrollDirection?: 'horizontal' | 'vertical' | 'both'
scrollToItem?: (Key) => void
shouldUseVirtualFocus?: boolean
sizeToFit?: 'width' | 'height'
transitionDuration?: number
}
useVirtualizerItemchanged by:
-useVirtualizerItem<T extends {}, V> {
- options: VirtualizerItemOptions<T, V>
- returnVal: undefined
-}
+VirtualizerItem-VirtualizerItem<T extends {}, V> {
- className?: string
- parent?: ReusableView<{}, V>
- reusableView: ReusableView<{}, V>
-}
+VirtualizerItemOptions-
+VirtualizerItemOptions {
+ layoutInfo: LayoutInfo
+ ref: RefObject<HTMLElement>
+ virtualizer: IVirtualizer
+}it changed:
undefined-
+useVirtualizerItem {
+ options: VirtualizerItemOptions
+ returnVal: undefined
+}undefined-
+VirtualizerItem {
+ children: ReactNode
+ className?: string
+ parent?: LayoutInfo
+}@react-spectrum/listboxListBox useListBoxLayout<T> {
state: ListState<T>
+ isLoading: boolean
returnVal: undefined
} |
snowystinger
approved these changes
Jun 1, 2023
reidbarber
approved these changes
Jun 1, 2023
This was referenced Jun 26, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #779. Includes and closes #4145, closes #4035, closes #4012, closes #4005, closes #3835, closes #3104
Note for reviewers: it may be helpful to review commit by commit. I've tried to separate the changes and included notes on each commit with details about the changes.
This fixes the remaining failing tests in React strict mode. Most of them were related to Virtualizer (the first two commits brought the failures from 745 to 157). Virtualizer is fixed by queuing updates to occur in a layout effect rather than updating during render, or in a raf. This way when double renders occur, we don't have two separate instances of the underlying Virtualizer class competing with one another.