Skip to content

Commit 702f528

Browse files
committed
refactor: scope PR to Autocomplete + LabelGroup render-derivation
Retargets base to main and drops the old #8001 leftovers (useMedia, SelectPanel, ToggleSwitch, ValidationAnimationContainer, Dialog, TreeView, SelectPanel2), which are handled by their own focused PRs. Keeps only the Autocomplete close-time re-sort and LabelGroup numeric-truncation render derivations.
1 parent 11214ba commit 702f528

9 files changed

Lines changed: 71 additions & 117 deletions

File tree

.changeset/derive-state-from-effects.md

Lines changed: 0 additions & 5 deletions
This file was deleted.

packages/react/src/Dialog/Dialog.tsx

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -492,7 +492,6 @@ const Buttons: React.FC<React.PropsWithChildren<{buttons: DialogButtonProps[]}>>
492492
if (hasRendered === 1) {
493493
autoFocusRef.current?.focus()
494494
} else {
495-
// Counts commits so focus is deferred by one render (a commit-timing hack), not derivable.
496495
// eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-derived-state
497496
setHasRendered(hasRendered + 1)
498497
}

packages/react/src/SelectPanel/SelectPanel.tsx

Lines changed: 6 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -245,16 +245,11 @@ function Panel({
245245
isSingleSelectModal ? selected : undefined,
246246
)
247247

248-
// Reset the intermediate selected item when the panel is opened or closed.
249-
// Tracking the previous `open` value in state lets us derive this during render
250-
// instead of in an effect. Adjusting state during render this way does not cause
251-
// an extra committed render — React re-renders synchronously before painting —
252-
// and a ref cannot be used here because writing refs during render is disallowed.
253-
const [prevOpen, setPrevOpen] = useState(open)
254-
if (prevOpen !== open) {
255-
setPrevOpen(open)
248+
// Reset the intermediate selected item when the panel is open/closed
249+
useEffect(() => {
250+
// eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-derived-state
256251
setIntermediateSelected(isSingleSelectModal ? selected : undefined)
257-
}
252+
}, [isSingleSelectModal, open, selected])
258253

259254
const onListContainerRefChanged: FilteredActionListProps['onListContainerRefChanged'] = useCallback(
260255
(node: HTMLElement | null) => {
@@ -397,8 +392,7 @@ function Panel({
397392
if (open) {
398393
// eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler
399394
if (items.length === 0 && !(isLoading || loading)) {
400-
// We need to wait for the listContainerElement to disappear before announcing no items,
401-
// otherwise it will be interrupted — this depends on commit timing, so it must run in an effect.
395+
// we need to wait for the listContainerElement to disappear before announcing no items, otherwise it will be interrupted
402396
// eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-adjust-state-on-prop-change
403397
setNeedsNoItemsAnnouncement(true)
404398
}
@@ -479,8 +473,7 @@ function Panel({
479473
// Only trigger filter change event if there are no items
480474
// eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler
481475
if (items.length === 0) {
482-
// Trigger filter event to populate panel on first open. This calls a consumer callback
483-
// (a side effect), so it must run in an effect rather than during render.
476+
// Trigger filter event to populate panel on first open
484477
// eslint-disable-next-line react-hooks/set-state-in-effect
485478
onFilterChange(filterValue, null)
486479
}

packages/react/src/ToggleSwitch/ToggleSwitch.tsx

Lines changed: 5 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -123,16 +123,12 @@ const ToggleSwitch = React.forwardRef<HTMLButtonElement, ToggleSwitchProps>(func
123123
}
124124
}, [onChange, checked, isControlled, disabled])
125125

126-
// Hiding the loading label is pure state derived from `loading`, so compute it
127-
// during render. Only the delayed reveal needs a timer, which stays in the
128-
// effect below.
129-
if (!loading && isLoadingLabelVisible) {
130-
setIsLoadingLabelVisible(false)
131-
}
132-
133126
useEffect(() => {
134-
// eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler
135-
if (loading && !isLoadingLabelVisible) {
127+
if (!loading && isLoadingLabelVisible) {
128+
// eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-chain-state-updates
129+
setIsLoadingLabelVisible(false)
130+
// eslint-disable-next-line react-you-might-not-need-an-effect/no-event-handler
131+
} else if (loading && !isLoadingLabelVisible) {
136132
safeSetTimeout(() => {
137133
setIsLoadingLabelVisible(true)
138134
}, loadingLabelDelay)

packages/react/src/TreeView/TreeView.tsx

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -556,8 +556,6 @@ const SubTree: FCWithSlotMarker<TreeViewSubTreeProps> = ({count, state, children
556556
const parentElement = document.getElementById(itemId)
557557
if (!parentElement) return
558558

559-
// `getAccessibleName` reads the committed DOM, so the label cannot be derived during
560-
// render and must be set from an effect.
561559
// eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-chain-state-updates
562560
setSubTreeLabel(getAccessibleName(parentElement))
563561
if (previousState === 'loading' && state === 'done') {

packages/react/src/experimental/SelectPanel2/SelectPanel.tsx

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -329,8 +329,6 @@ const SelectPanelButton = React.forwardRef<HTMLButtonElement, ButtonProps>((prop
329329
useEffect(() => {
330330
const label = document.querySelector(`[for='${inputProps.id}']`)
331331
if (label?.textContent) {
332-
// Reads the associated label's `textContent` from the committed DOM, so it must run in
333-
// an effect.
334332
// eslint-disable-next-line react-hooks/set-state-in-effect
335333
setLabelText(label.textContent)
336334
}

packages/react/src/hooks/__tests__/useMedia.test.tsx

Lines changed: 1 addition & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,11 @@ type MediaQueryEventListener = (event: {matches: boolean}) => void
88

99
function mockMatchMedia({defaultMatch = false} = {}) {
1010
const listeners = new Set<MediaQueryEventListener>()
11-
// Track the current match state so that reading `matchMedia(query).matches`
12-
// reflects the latest value, mirroring real `MediaQueryList` behavior.
13-
let currentMatches = defaultMatch
1411

1512
Object.defineProperty(window, 'matchMedia', {
1613
writable: true,
1714
value: vi.fn().mockImplementation(query => ({
18-
matches: currentMatches,
15+
matches: defaultMatch,
1916
media: query,
2017
onchange: null,
2118
addListener: vi.fn(), // deprecated
@@ -32,7 +29,6 @@ function mockMatchMedia({defaultMatch = false} = {}) {
3229

3330
return {
3431
change({matches = false}) {
35-
currentMatches = matches
3632
for (const listener of listeners) {
3733
listener({
3834
matches,
@@ -76,32 +72,10 @@ describe('useMedia', () => {
7672
return null
7773
}
7874

79-
// `renderToString` uses the server snapshot, which defaults to `false` when
80-
// no `defaultState` is provided.
8175
ReactDOM.renderToString(<TestComponent />)
8276
expect(match[0]).toBe(false)
8377
})
8478

85-
it('does not warn about a missing defaultState on the client', () => {
86-
const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {})
87-
88-
function TestComponent() {
89-
useMedia('(pointer: coarse)')
90-
return null
91-
}
92-
93-
// On the client (where `window` is defined) the hook reads `matchMedia`
94-
// directly, so the SSR guidance warning must not appear in the browser
95-
// console — including during hydration, when `getServerSnapshot` runs.
96-
render(<TestComponent />)
97-
98-
expect(warnSpy).not.toHaveBeenCalledWith(
99-
'Warning:',
100-
expect.stringContaining('`useMedia` When server side rendering'),
101-
)
102-
warnSpy.mockRestore()
103-
})
104-
10579
it('should respond to change in matchMedia values', () => {
10680
const {change} = mockMatchMedia()
10781

Lines changed: 54 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
1-
import {useCallback, useContext, useSyncExternalStore} from 'react'
1+
import React, {useContext, useEffect} from 'react'
2+
import {canUseDOM} from '../utils/environment'
23
import {warning} from '../utils/warning'
34
import {MatchMediaContext} from './MatchMediaContext'
45

@@ -17,69 +18,71 @@ import {MatchMediaContext} from './MatchMediaContext'
1718
*/
1819
export function useMedia(mediaQueryString: string, defaultState?: boolean) {
1920
const features = useContext(MatchMediaContext)
20-
// When the query is provided through `MatchMedia` context, that value always
21-
// wins and there is nothing external to subscribe to.
22-
const contextValue = features[mediaQueryString] as boolean | undefined
21+
const [matches, setMatches] = React.useState(() => {
22+
if (features[mediaQueryString] !== undefined) {
23+
return features[mediaQueryString] as boolean
24+
}
2325

24-
const subscribe = useCallback(
25-
(onStoreChange: () => void) => {
26-
if (contextValue !== undefined) {
27-
return () => {}
28-
}
26+
// Prevent a React hydration mismatch when a default value is provided by not defaulting to window.matchMedia(query).matches.
27+
if (defaultState !== undefined) {
28+
return defaultState
29+
}
2930

30-
const mediaQueryList = window.matchMedia(mediaQueryString)
31+
if (canUseDOM) {
32+
return window.matchMedia(mediaQueryString).matches
33+
}
3134

32-
// Support fallback to `addListener` for broader browser support
33-
// @ts-ignore this is not present in Safari <14
34-
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
35-
if (mediaQueryList.addEventListener) {
36-
mediaQueryList.addEventListener('change', onStoreChange)
37-
return () => {
38-
mediaQueryList.removeEventListener('change', onStoreChange)
39-
}
40-
}
35+
// A default value has not been provided, and you are rendering on the server, warn of a possible hydration mismatch when defaulting to false.
36+
warning(
37+
true,
38+
'`useMedia` When server side rendering, defaultState should be defined to prevent a hydration mismatches.',
39+
)
4140

42-
mediaQueryList.addListener(onStoreChange)
43-
return () => {
44-
mediaQueryList.removeListener(onStoreChange)
45-
}
46-
},
47-
[contextValue, mediaQueryString],
48-
)
41+
return false
42+
})
43+
44+
if (features[mediaQueryString] !== undefined && matches !== features[mediaQueryString]) {
45+
setMatches(features[mediaQueryString] as boolean)
46+
}
4947

50-
const getSnapshot = useCallback(() => {
51-
if (contextValue !== undefined) {
52-
return contextValue
48+
useEffect(() => {
49+
// If `mediaQueryString` is present in features through `context` defer to
50+
// the value present instead of checking with matchMedia
51+
if (features[mediaQueryString] !== undefined) {
52+
return
5353
}
54-
return window.matchMedia(mediaQueryString).matches
55-
}, [contextValue, mediaQueryString])
5654

57-
const getServerSnapshot = useCallback(() => {
58-
if (contextValue !== undefined) {
59-
return contextValue
55+
function listener(event: MediaQueryListEvent) {
56+
setMatches(event.matches)
6057
}
6158

62-
// Prevent a React hydration mismatch when a default value is provided by not
63-
// defaulting to `window.matchMedia(query).matches`.
64-
if (defaultState !== undefined) {
65-
return defaultState
59+
const mediaQueryList = window.matchMedia(mediaQueryString)
60+
61+
// Support fallback to `addListener` for broader browser support
62+
// @ts-ignore this is not present in Safari <14
63+
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
64+
if (mediaQueryList.addEventListener) {
65+
mediaQueryList.addEventListener('change', listener)
66+
} else {
67+
mediaQueryList.addListener(listener)
6668
}
6769

68-
// A default value has not been provided, and you are rendering on the
69-
// server, warn of a possible hydration mismatch when defaulting to false.
70-
// `getServerSnapshot` also runs on the client during hydration; only warn on
71-
// the actual server so we don't surface this message in the browser console
72-
// (matching the previous behavior, where the client read `matchMedia` and
73-
// stayed quiet).
74-
warning(
75-
typeof window === 'undefined',
76-
'`useMedia` When server side rendering, defaultState should be defined to prevent a hydration mismatches.',
77-
)
70+
// Make sure the media query list is in sync with the matches state
71+
// eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-external-store-subscription
72+
setMatches(mediaQueryList.matches)
7873

79-
return false
80-
}, [contextValue, defaultState])
74+
return () => {
75+
// @ts-ignore this is not present in Safari <14
76+
// eslint-disable-next-line @typescript-eslint/no-unnecessary-condition
77+
if (mediaQueryList.addEventListener) {
78+
mediaQueryList.removeEventListener('change', listener)
79+
} else {
80+
mediaQueryList.removeListener(listener)
81+
}
82+
}
83+
}, [features, mediaQueryString])
8184

82-
return useSyncExternalStore(subscribe, getSnapshot, getServerSnapshot)
85+
return matches
8386
}
8487

8588
export {MatchMedia} from './MatchMedia'

packages/react/src/internal/components/ValidationAnimationContainer.tsx

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type {HTMLProps} from 'react'
22
import type React from 'react'
3-
import {useState} from 'react'
3+
import {useEffect, useState} from 'react'
44
import classes from './ValidationAnimationContainer.module.css'
55

66
interface Props extends HTMLProps<HTMLDivElement> {
@@ -9,12 +9,10 @@ interface Props extends HTMLProps<HTMLDivElement> {
99
const ValidationAnimationContainer: React.FC<React.PropsWithChildren<Props>> = ({show, children}) => {
1010
const [shouldRender, setRender] = useState(show)
1111

12-
// Mounting is derived from `show`, so compute it during render. Un-mounting is
13-
// deferred to `onAnimationEnd` so the exit animation can play. Deriving this in
14-
// render avoids the extra commit/paint an effect + setState would introduce.
15-
if (show && !shouldRender) {
16-
setRender(true)
17-
}
12+
useEffect(() => {
13+
// eslint-disable-next-line react-hooks/set-state-in-effect, react-you-might-not-need-an-effect/no-derived-state, react-you-might-not-need-an-effect/no-event-handler
14+
if (show) setRender(true)
15+
}, [show])
1816

1917
const onAnimationEnd = () => {
2018
if (!show) setRender(false)

0 commit comments

Comments
 (0)