Skip to content

Fix React Strictmode for sliders #4012

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

Closed
wants to merge 13 commits into from
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/@react-aria/slider/src/useSlider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {AriaSliderProps} from '@react-types/slider';
import {clamp, mergeProps, useGlobalListeners} from '@react-aria/utils';
import {DOMAttributes} from '@react-types/shared';
import {getSliderThumbId, sliderIds} from './utils';
import React, {LabelHTMLAttributes, OutputHTMLAttributes, RefObject, useRef} from 'react';
import React, {LabelHTMLAttributes, OutputHTMLAttributes, RefObject, useEffect, useRef} from 'react';
import {setInteractionModality, useMove} from '@react-aria/interactions';
import {SliderState} from '@react-stately/slider';
import {useLabel} from '@react-aria/label';
Expand Down Expand Up @@ -67,7 +67,9 @@ export function useSlider<T extends number | number[]>(
const realTimeTrackDraggingIndex = useRef<number | null>(null);

const stateRef = useRef<SliderState>(null);
stateRef.current = state;
useEffect(() => {
stateRef.current = state;
});
const reverseX = direction === 'rtl';
const currentPosition = useRef<number>(null);
const {moveProps} = useMove({
Expand Down
4 changes: 3 additions & 1 deletion packages/@react-aria/slider/src/useSliderThumb.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,9 @@ export function useSliderThumb(
}, [isFocused, focusInput]);

const stateRef = useRef<SliderState>(null);
stateRef.current = state;
useEffect(() => {
stateRef.current = state;
});
Copy link
Member

Choose a reason for hiding this comment

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

We can get rid of stateRef completely if we make useMove use useEffectEvent. Done in #4564.

let reverseX = direction === 'rtl';
let currentPosition = useRef<number>(null);

Expand Down
13 changes: 8 additions & 5 deletions packages/@react-spectrum/slider/test/RangeSlider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {fireEvent, render} from '@react-spectrum/test-utils';
import {press, testKeypresses} from './utils';
import {Provider} from '@adobe/react-spectrum';
import {RangeSlider} from '../';
import React, {useState} from 'react';
import React, {useCallback, useState} from 'react';
import {theme} from '@react-spectrum/theme-default';
import userEvent from '@testing-library/user-event';

Expand Down Expand Up @@ -126,11 +126,14 @@ describe('RangeSlider', function () {
});

it('can be controlled', function () {
let renders = [];
let setValues = [];

function Test() {
let [value, setValue] = useState({start: 20, end: 40});
renders.push(value);
let [value, _setValue] = useState({start: 20, end: 40});
let setValue = useCallback((val) => {
setValues.push(val);
_setValue(val);
}, [_setValue]);

return (<RangeSlider label="The Label" value={value} onChange={setValue} />);
}
Expand All @@ -154,7 +157,7 @@ describe('RangeSlider', function () {
expect(sliderRight).toHaveAttribute('aria-valuetext', '50');
expect(output).toHaveTextContent('30 – 50');

expect(renders).toStrictEqual([{start: 20, end: 40}, {start: 30, end: 40}, {start: 30, end: 50}]);
expect(setValues).toStrictEqual([{start: 30, end: 40}, {start: 30, end: 50}]);
});

it('supports a custom valueLabel', function () {
Expand Down
13 changes: 8 additions & 5 deletions packages/@react-spectrum/slider/test/Slider.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import {act, fireEvent, installMouseEvent, render} from '@react-spectrum/test-utils';
import {press, testKeypresses} from './utils';
import {Provider} from '@adobe/react-spectrum';
import React, {useState} from 'react';
import React, {useCallback, useState} from 'react';
import {Slider} from '../';
import {theme} from '@react-spectrum/theme-default';
import userEvent from '@testing-library/user-event';
Expand Down Expand Up @@ -119,11 +119,14 @@ describe('Slider', function () {
});

it('can be controlled', function () {
let renders = [];
let setValues = [];

function Test() {
let [value, setValue] = useState(50);
renders.push(value);
let [value, _setValue] = useState(50);
let setValue = useCallback((val) => {
setValues.push(val);
_setValue(val);
}, [_setValue]);

return (<Slider label="The Label" value={value} onChange={setValue} />);
}
Expand All @@ -141,7 +144,7 @@ describe('Slider', function () {
expect(slider).toHaveAttribute('aria-valuetext', '55');
expect(output).toHaveTextContent('55');

expect(renders).toStrictEqual([50, 55]);
expect(setValues).toStrictEqual([55]);
});

it('supports a custom getValueLabel', function () {
Expand Down
8 changes: 5 additions & 3 deletions packages/@react-stately/slider/src/useSliderState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {clamp, snapValueToStep} from '@react-aria/utils';
import {Orientation} from '@react-types/shared';
import {SliderProps} from '@react-types/slider';
import {useControlledState} from '@react-stately/utils';
import {useMemo, useRef, useState} from 'react';
import {useEffect, useMemo, useRef, useState} from 'react';

export interface SliderState {
/**
Expand Down Expand Up @@ -192,9 +192,11 @@ export function useSliderState<T extends number | number[]>(props: SliderStateOp
const [focusedIndex, setFocusedIndex] = useState<number | undefined>(undefined);

const valuesRef = useRef<number[]>(null);
valuesRef.current = values;
const isDraggingsRef = useRef<boolean[]>(null);
isDraggingsRef.current = isDraggings;
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.

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

Copy link
Member Author

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.

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.

valuesRef.current = values;
isDraggingsRef.current = isDraggings;
});

function getValuePercent(value: number) {
return (value - minValue) / (maxValue - minValue);
Expand Down
46 changes: 26 additions & 20 deletions packages/@react-stately/utils/src/useControlledState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,46 +10,49 @@
* governing permissions and limitations under the License.
*/

import {useCallback, useRef, useState} from 'react';
import {useCallback, useEffect, useRef, useState} from 'react';

export function useControlledState<T>(
value: T,
defaultValue: T,
onChange: (value: T, ...args: any[]) => void
): [T, (value: T, ...args: any[]) => void] {
let [stateValue, setStateValue] = useState(value || defaultValue);
let ref = useRef(value !== undefined);
let wasControlled = ref.current;
let isControlled = value !== undefined;
// Internal state reference for useCallback
let stateRef = useRef(stateValue);
if (wasControlled !== isControlled) {
console.warn(`WARN: A component changed from ${wasControlled ? 'controlled' : 'uncontrolled'} to ${isControlled ? 'controlled' : 'uncontrolled'}.`);
}

ref.current = isControlled;
// detect if a component is switching from controlled to uncontrolled or vice versa, warn if so
let isControlledRef = useRef(isControlled);
useEffect(() => {
let wasControlled = isControlledRef.current;
if (isControlled !== wasControlled) {
console.warn(`WARN: A component changed from ${wasControlled ? 'controlled' : 'uncontrolled'} to ${isControlled ? 'controlled' : 'uncontrolled'}.`);
}
isControlledRef.current = isControlled;
}, [value, isControlled]);

let setValue = useCallback((value, ...args) => {
let onChangeCaller = (value, ...onChangeArgs) => {
let setValue = useCallback((newValue, ...args) => {
let onChangeCaller = (updateValue, ...onChangeArgs) => {
if (onChange) {
if (!Object.is(stateRef.current, value)) {
onChange(value, ...onChangeArgs);
if (!Object.is(stateRef.current, updateValue)) {
onChange(updateValue, ...onChangeArgs);
}
}
if (!isControlled) {
stateRef.current = value;
stateRef.current = updateValue;
}
};

if (typeof value === 'function') {
if (typeof newValue === 'function') {
console.warn('We can not support a function callback. See Github Issues for details https://github.com/adobe/react-spectrum/issues/2320');
// this supports functional updates https://reactjs.org/docs/hooks-reference.html#functional-updates
// when someone using useControlledState calls setControlledState(myFunc)
// this will call our useState setState with a function as well which invokes myFunc and calls onChange with the value from myFunc
// if we're in an uncontrolled state, then we also return the value of myFunc which to setState looks as though it was just called with myFunc from the beginning
// otherwise we just return the controlled value, which won't cause a rerender because React knows to bail out when the value is the same
let updateFunction = (oldValue, ...functionArgs) => {
let interceptedValue = value(isControlled ? stateRef.current : oldValue, ...functionArgs);
let interceptedValue = newValue(isControlled ? stateRef.current : oldValue, ...functionArgs);
onChangeCaller(interceptedValue, ...args);
if (!isControlled) {
return interceptedValue;
Expand All @@ -59,16 +62,19 @@ export function useControlledState<T>(
setStateValue(updateFunction);
} else {
if (!isControlled) {
setStateValue(value);
setStateValue(newValue);
}
onChangeCaller(value, ...args);
onChangeCaller(newValue, ...args);
}
}, [isControlled, onChange]);

// If a controlled component's value prop changes, we need to update stateRef
if (isControlled) {
stateRef.current = value;
} else {
useEffect(() => {
// If a controlled component's value prop changes, we need to update stateRef
if (isControlled) {
stateRef.current = value;
}
});
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Went back and checked the tests passing before and after. It's the same. So changing useControlledState was just following the linter.

if (!isControlled) {
value = stateValue;
}

Expand Down
8 changes: 6 additions & 2 deletions packages/@react-stately/utils/test/useControlledState.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,12 +99,16 @@ describe('useControlledState tests', function () {
let {getByRole, getByTestId} = render(<TestComponentWrapper defaultValue={5} />);
let button = getByRole('button');
getByTestId('5');
expect(renderSpy).toBeCalledTimes(1);
if (!process.env.STRICT_MODE) {
expect(renderSpy).toBeCalledTimes(1);
}
actDOM(() =>
userEvent.click(button)
);
getByTestId('6');
expect(renderSpy).toBeCalledTimes(2);
if (!process.env.STRICT_MODE) {
expect(renderSpy).toBeCalledTimes(2);
}
});

it('can handle controlled setValue behavior', () => {
Expand Down