-
Notifications
You must be signed in to change notification settings - Fork 1.4k
chore: Update eslint plugin react hooks immutability #9055
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 reverts commit 7b96414.
# Conflicts: # yarn.lock
# Conflicts: # packages/@react-stately/checkbox/src/useCheckboxGroupState.ts
…ugin-react-hooks-followup
…eslint-plugin-react-hooks-immutability
| }, [document]); | ||
| let getServerSnapshot = useCallback(() => { | ||
| document.isSSR = true; | ||
| document.setSSR(true); |
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.
had to introduce this because the React compiler is a bit strict about mutating, I think it should be allowed in this case, but React doesn't know that, so I've tricked it. I had tried storing the document in a ref and editing it that way, which fixes the compiler problem, but it broke some tests, I'm not sure why and it makes me a little worried. This is the closest to what we've tested for years though, so I'm ok with this for now.
|
|
||
| let componentIds = new WeakMap(); | ||
|
|
||
| function useCounter(isDisabled = false) { |
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.
moved these things that can't be readily fixed to other files so the compiler won't bail on the entire file
|
|
||
| // Syncs ref from context with ref passed to hook | ||
| export function useSyncRef<T>(context?: ContextValue<T> | null, ref?: RefObject<T | null>): void { | ||
| export function useSyncRef<T>(contextRef?: RefObject<T | null> | null, ref?: RefObject<T | null>): void { |
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.
private function so I'm ok changing the API
however, if we're worried, I can make a second one which is safe for the compiler vs this old one
| }; | ||
| } | ||
|
|
||
| function useScrollViewContentSizeChange({updateSize, contentSize, isUpdatingSize, setUpdate}: {updateSize: (flush: typeof flushSync) => void, contentSize: Size, isUpdatingSize: RefObject<boolean>, setUpdate: (update: {}) => void}) { |
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.
may need to move to a separate file, can't readily fix this one
| granularity, | ||
| hasTime, | ||
| setDate(part, date) { | ||
| // Both the start and end datefields update on form reset back to back, so this is a problem due to not supporting setState callbacks |
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.
comment a little out of date with changes we made
| let value = controlledValue || placeholderValue; | ||
|
|
||
| let setValue = (newValue: RangeValue<DateValue | null> | null) => { | ||
| // TODO: I don't know how to fix this one |
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 tried 3 times to re-write this file, I failed each time. Anyone else want a swing at it?
…ugin-react-hooks-followup
…ugin-react-hooks-followup # Conflicts: # packages/react-aria-components/stories/ListBox.stories.tsx
…eslint-plugin-react-hooks-immutability
# Conflicts: # eslint.config.mjs
…eslint-plugin-react-hooks-immutability
|
Build successful! 🎉 |
# Conflicts: # eslint.config.mjs
|
Build successful! 🎉 |
|
Build successful! 🎉 |
# Conflicts: # packages/@react-spectrum/s2/test/EditableTableView.test.tsx
|
Build successful! 🎉 |
# Conflicts: # packages/react-aria-components/src/Dialog.tsx
|
Build successful! 🎉 |
## API Changes
@react-aria/utils/@react-aria/utils:useSyncRef useSyncRef <T> {
- context?: ContextValue<T> | null
+ contextRef?: RefObject<T | null> | null
ref?: RefObject<T | null>
returnVal: undefined
} |
| ); | ||
| } | ||
| }, [calendar, granularity, validSegments, defaultTimeZone, props.placeholderValue]); | ||
| let [lastCalendar, setLastCalendar] = useState(calendar); |
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.
what's the reason for changing this? when i ran the lint, i didn't see any errors regarding these lines
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.
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.
could this be a potential false positive? saw this while looking into it more facebook/react#34045
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 believe it falls into this category facebook/react#34045 (comment)
And the extra setState will be batched with the placeholder change anyways
it'll also now be more precise to the state as it'll bail and then render again with the up to date state, meaning no possible flash of stale state
| buttonGroup: {UNSAFE_className: classNames(styles, 'spectrum-Dialog-buttonGroup', {'spectrum-Dialog-buttonGroup--noFooter': !hasFooter}), align: 'end'} | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }), [hasFooter, hasHeader, titleProps]); | ||
| }), [hasFooter, hasHeader, titleProps, hasTypeIcon, hasHeading]); |
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.
was there a reason why we disabled the lint here instead of just adding all the dependencies in the first place? this is probably fine, im just curious if we intentionally left it out for some reason
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 suspect that the line was eslint disabled to suppress a warning because we were lazy, including these is correct
Change that introduced it agreed upon ignoring to cut down on total number of warnings with least changes:
ed2c240

Closes
This PR focuses on react-hooks/immutability and removing a few eslint-disable-next-lines
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: