Skip to content

Commit

Permalink
fix(useCustomEnsuredControl): Update useCustomEnsuredControl types to…
Browse files Browse the repository at this point in the history
… reveal prevValue undefined (#7285)

Выяснилось, что всё-таки, если передавать в свойство `onChange` `useCustomEnsuredControl` коллбэк `nextValue` c аргументом `prevValue`, то есть вероятность, что `prevValue` будет `undefined`.
Это видно, если исправить тип аргумента prevValue с `any` на `V`.
https://github.com/VKCOM/VKUI/blob/a7b79b3b3a69bdf68163f62521f68697aceb52c2/packages/vkui/src/hooks/useEnsuredControl.ts#L56

Дело в том, что `preservedControlledValueRef`, которое мы передаём в `nextValue` коллбэк как значение `prevValue`, может быть `undefined`, так как хранит предыдущее значение `value`, которое тоже может быть `undefined`.
https://github.com/VKCOM/VKUI/blob/a7b79b3b3a69bdf68163f62521f68697aceb52c2/packages/vkui/src/hooks/useEnsuredControl.ts#L70-L73

В теории (потому что в тестах не удалось повторить), если пользователь в `useCustomEnsuredControl` не которолирует значение `value` (то есть проп `value === undefined`, а мы храним значение `value` локально в `useCustomEnsuredControl`), а затем делает `value` котролируемым (то есть начинает передавать в проп `value` какое-то значение), то есть вероятность, что при вызове `onChanage` `preservedControlledValueRef` ещё не обновился и равен предыдущему значение `value`, то есть `undefined`, тогда пользователь получит `prevValue` со значением `undefined`, что может привести к ошибке, как описано в #7099
> TypeError: Uncaught TypeError: Cannot read properties of undefined (reading 'shown')

## Изменения
- Исправили тип `prevValue` в колбэке `nextValue`, заменив `any`, чтобы была видна проблема c undefined.

Но теперь ясно, что мы не можем просто так передать `preservedControlledValueRef` в `nextValue` коллбэк в качестве `prevValue`, потому что типы не сходятся.

Что можно сделать.

1. Просто не вызывать `nextValue`, если `preservedControlledValueRef` `undefined`, но тогда мы получаем риск не отреагировать на действие пользователя, так как `onChange` не будет вызван и состояние не поменяется.

2. Разрешить `prevValue` по типам быть `undefined`. Но тогда пользователи, использующие в `onChange` коллбэк `nextValue` получат по типам вариант, что `prevValue` может быть `undefined`.
Это очень странное состояние и тяжело объяснить его причины. А также тяжело принять решение при обработке onChange.
Например, в `useChipsInput` не понятно откуда тогда взять актуальный массив `chips`, чтобы добавить в него новое значение.
https://github.com/VKCOM/VKUI/blob/7755cabb003c2dac894703ac30335c0b874d8d52/packages/vkui/src/components/ChipsInput/useChipsInput.ts#L107-L126

3. (Текущее решение) Если `preservedControlledValueRef` `undefined`, то использовать значение `value`.
Так как мы знаем, что компонент котролируемый, значит `value` имеет какое-то значение. При переходе из некоторолируемого значения в контролируемое у `value` нету предыдущего состояния, поэтому самым адекватным решением будет предать текущее значение `value` как `prevValue`.
Чтобы `value` не находилось в массиве зависимостей `onChange` и не тригерило новое значение `onChange` при изменении, мы `value` прячем в `ref`, и используем лишь тогда, когда `preservedControlledValueRef` === `undefined`.

- Добавили warning. Хотелось ещё дать ссылку на [React warning](https://react.dev/reference/react-dom/components/input#im-getting-an-error-a-component-is-changing-an-uncontrolled-input-to-be-controlled), тогда надо пояснять, что в нашем случае речь не только про input, а как это сделать лаконично я не придумал.
  • Loading branch information
mendrew authored and actions-user committed Aug 22, 2024
1 parent b235f82 commit 6ab8c39
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 5 deletions.
42 changes: 38 additions & 4 deletions packages/vkui/src/hooks/useEnsuredControl.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as React from 'react';
import { isFunction } from '@vkontakte/vkjs';
import { useIsomorphicLayoutEffect } from '../lib/useIsomorphicLayoutEffect';
import { warnOnce } from '../lib/warnOnce';

export interface UseEnsuredControlProps<V, E extends React.ChangeEvent<any>> {
value?: V;
Expand Down Expand Up @@ -38,6 +39,8 @@ export interface UseCustomEnsuredControlProps<V> {
onChange?: (this: void, v: V) => any;
}

const warn = warnOnce('useCustomEnsuredControl');

export function useCustomEnsuredControl<V = any>({
value,
defaultValue,
Expand All @@ -46,14 +49,28 @@ export function useCustomEnsuredControl<V = any>({
}: UseCustomEnsuredControlProps<V>): [V, React.Dispatch<React.SetStateAction<V>>] {
const isControlled = value !== undefined;
const [localValue, setLocalValue] = React.useState(defaultValue);
const preservedControlledValueRef = React.useRef(value);

const preservedControlledValueRef = React.useRef<V | undefined>();
useIsomorphicLayoutEffect(() => {
preservedControlledValueRef.current = value;
});

/*
* Для ситуации, когда nextValue это пользовательская функция,
* и в качестве аргумента мы должны передать prevValue.
* Обычно в качестве prevValue используется preservedControlledValueRef, но оно может быть undefined, если
* некотролируемое value вдруг стало контролируемым
* (value = undefined ---> value = true)
* Если в момент вызова onChange preservedControlledValueRef ещё не был
* обновлён в useEffect, то мы не можем использовать preservedControlledValueRef как prevValue
* В качестве запасного варианта мы храним текущее значение value в currentFallbackValueRef, чтобы
* использовать его вместо preservedControlledValueRef.
*/
const currentFallbackValueRef = React.useRef<V | undefined>(value);
currentFallbackValueRef.current = value;

const onChange = React.useCallback(
(nextValue: V | ((prevValue: any) => V)) => {
(nextValue: React.SetStateAction<V>) => {
if (disabled) {
return;
}
Expand All @@ -68,8 +85,25 @@ export function useCustomEnsuredControl<V = any>({
return resolvedValue;
});
} else if (onChangeProp) {
const resolvedValue = nextValue(preservedControlledValueRef.current);
onChangeProp(resolvedValue);
if (process.env.NODE_ENV === 'development') {
if (preservedControlledValueRef.current === undefined) {
warn(
`Похоже, что при вызове onChange с аргументом nextValue в виде коллбэка, состояние компонента было переведено из неконтролируемого ("undefined") в контролируемое. Пожалуйста, старайтесь сохранять либо неконтролируемое состояние, либо контролируемое на всём промежутке жизненного цикла компонента, чтобы получать предсказуемое значение prevValue в коллбэке nextValue((prevValue: V) => V)`,
'error',
);
}
}

const prevValue =
preservedControlledValueRef.current === undefined
? currentFallbackValueRef.current
: preservedControlledValueRef.current;
// В теории prevValue не может быть undefined,
// но лучше не вызывать nextValue с таким значением
if (prevValue !== undefined) {
const resolvedValue = nextValue(prevValue);
onChangeProp(resolvedValue);
}
}
} else {
if (onChangeProp) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ export const useFloatingWithInteractions = <T extends HTMLElement = HTMLElement>
onShownChange: onShownChangeProp,
onShownChanged: onShownChangedProp,
}: UseFloatingWithInteractionsProps): UseFloatingWithInteractionsReturn<T> => {
const memoizedValue = React.useMemo(
const memoizedValue = React.useMemo<LocalState | undefined>(
() => (shownProp !== undefined ? { shown: shownProp } : undefined),
[shownProp],
);
Expand Down

0 comments on commit 6ab8c39

Please sign in to comment.