Skip to content

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented Oct 17, 2025

Closes

This PR focused on these rules
'react-hooks/globals': ERROR,
'react-hooks/purity': ERROR,
'react-hooks/set-state-in-effect': ERROR,
'react-hooks/static-components': ERROR,

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

🧢 Your Project:

@snowystinger snowystinger changed the title Update eslint plugin react hooks followup chore: Update eslint plugin react hooks followup Oct 17, 2025
@rspbot
Copy link

rspbot commented Oct 17, 2025

@adobe adobe deleted a comment from rspbot Oct 17, 2025

// Use a random drag type so the items can only be reordered within this list and not dragged elsewhere.
let dragType = React.useMemo(() => `keys-${Math.random().toString(36).slice(2)}`, []);
let dragType = React.useMemo(() => randomDragTypeReorderExample, []);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there's a lot of these, i just moved the random generator out of the component, that should be fine because you couldn't drag between different examples this way either and I don't think we render the same one twice as an example

ref2 = useRef(null);
let internalRef1 = useRef(null);
let internalRef2 = useRef(null);
useEffect(() => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

other option i guess would be to exclude the compiler from running on test files, but seems better to always be in good habits

}, [tablistRef, wrapperRef, direction, orientation, setCollapsed, prevTabPositions, setTabPositions]);

useEffect(() => {
useLayoutEffect(() => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure why this was an effect, it's meant to measure the dom, so layout effect should be more correct

}
});

// TODO: Lint doesn't catch these, it thinks we're not in a component render cycle here?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what to make of this yet, but not going to handle it in this series of PRs

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kinda weird that it even catches the other ones, the hook that is being called doesn't create a component like https://react.dev/reference/eslint-plugin-react-hooks/lints/static-components demonstrates but perhaps I'm not understanding quite what React is seeing here. To be safe we could define one for table, tr, td, tbody, and th which should cover it I think

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, went ahead and updated them

I'm mostly worried that the compiler isn't looking through these components, so won't do any optimisations
I'll raise an issue with React at some point about it probably

Copy link
Member

@LFDanLu LFDanLu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other changes look good to me, just one comment

}
});

// TODO: Lint doesn't catch these, it thinks we're not in a component render cycle here?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

kinda weird that it even catches the other ones, the hook that is being called doesn't create a component like https://react.dev/reference/eslint-plugin-react-hooks/lints/static-components demonstrates but perhaps I'm not understanding quite what React is seeing here. To be safe we could define one for table, tr, td, tbody, and th which should cover it I think

@rspbot
Copy link

rspbot commented Oct 19, 2025

@rspbot
Copy link

rspbot commented Oct 19, 2025

@rspbot
Copy link

rspbot commented Oct 19, 2025

## API Changes

react-aria-components

/react-aria-components:RangeCalendarState

-RangeCalendarState <T extends DateValue = DateValue> {
+RangeCalendarState {
   anchorDate: CalendarDate | null
   focusNextDay: () => void
   focusNextPage: () => void
   focusNextSection: (boolean) => void
   focusPreviousDay: () => void
   focusPreviousPage: () => void
   focusPreviousRow: () => void
   focusPreviousSection: (boolean) => void
   focusSectionEnd: () => void
   focusSectionStart: () => void
   focusedDate: CalendarDate
   getDatesInWeek: (number, CalendarDate) => Array<CalendarDate | null>
   highlightDate: (CalendarDate) => void
   highlightedRange: RangeValue<CalendarDate> | null
   isCellDisabled: (CalendarDate) => boolean
   isCellFocused: (CalendarDate) => boolean
   isCellUnavailable: (CalendarDate) => boolean
   isDisabled: boolean
   isDragging: boolean
   isFocused: boolean
   isInvalid: (CalendarDate) => boolean
   isNextVisibleRangeInvalid: () => boolean
   isPreviousVisibleRangeInvalid: () => boolean
   isReadOnly: boolean
   isSelected: (CalendarDate) => boolean
   isValueInvalid: boolean
   maxValue?: DateValue | null
   minValue?: DateValue | null
   selectDate: (CalendarDate) => void
   selectFocusedDate: () => void
   setAnchorDate: (CalendarDate | null) => void
   setDragging: (boolean) => void
   setFocused: (boolean) => void
   setFocusedDate: (CalendarDate) => void
   setValue: (RangeValue<DateValue> | null) => void
   timeZone: string
   value: RangeValue<DateValue> | null
   visibleRange: RangeValue<CalendarDate>
 }

@react-aria/utils

/@react-aria/utils:useUpdateEffect

 useUpdateEffect {
-  effect: EffectCallback
+  cb: EffectCallback
   dependencies: Array<any>
   returnVal: undefined
 }

@react-stately/calendar

/@react-stately/calendar:RangeCalendarState

-RangeCalendarState <T extends DateValue = DateValue> {
+RangeCalendarState {
   anchorDate: CalendarDate | null
   focusNextDay: () => void
   focusNextPage: () => void
   focusNextSection: (boolean) => void
   focusPreviousDay: () => void
   focusPreviousPage: () => void
   focusPreviousRow: () => void
   focusPreviousSection: (boolean) => void
   focusSectionEnd: () => void
   focusSectionStart: () => void
   focusedDate: CalendarDate
   getDatesInWeek: (number, CalendarDate) => Array<CalendarDate | null>
   highlightDate: (CalendarDate) => void
   highlightedRange: RangeValue<CalendarDate> | null
   isCellDisabled: (CalendarDate) => boolean
   isCellFocused: (CalendarDate) => boolean
   isCellUnavailable: (CalendarDate) => boolean
   isDisabled: boolean
   isDragging: boolean
   isFocused: boolean
   isInvalid: (CalendarDate) => boolean
   isNextVisibleRangeInvalid: () => boolean
   isPreviousVisibleRangeInvalid: () => boolean
   isReadOnly: boolean
   isSelected: (CalendarDate) => boolean
   isValueInvalid: boolean
   maxValue?: DateValue | null
   minValue?: DateValue | null
   selectDate: (CalendarDate) => void
   selectFocusedDate: () => void
   setAnchorDate: (CalendarDate | null) => void
   setDragging: (boolean) => void
   setFocused: (boolean) => void
   setFocusedDate: (CalendarDate) => void
   setValue: (RangeValue<DateValue> | null) => void
   timeZone: string
   value: RangeValue<DateValue> | null
   visibleRange: RangeValue<CalendarDate>
 }

if (prevIsLoading !== isLoading && !isLoading) {
setShowLoading(false);
setPrevIsLoading(isLoading);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this one felt weird to me as well, but https://react.dev/reference/react/useState#storing-information-from-previous-renders claims it is better than setting state in an effect (which the linter will now complain about)

}, [leftOffset, popoverContainer]);
let [prevPopoverContainer, setPrevPopoverContainer] = useState<HTMLElement | null>(null);
if (popoverContainer && prevPopoverContainer !== popoverContainer && leftOffset.left === 0) {
let {left} = popoverContainer.getBoundingClientRect();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading from the DOM during render?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crud, i missed that one, i'll look at that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants