Skip to content

fix(useCustomEnsuredControl): Update useCustomEnsuredControl types to reveal prevValue undefined #7285

Merged
mendrew merged 6 commits intomasterfrom
mendrew/fix/useCustomEnsuredControl/possible-undefined-pre-value
Aug 22, 2024
Merged

fix(useCustomEnsuredControl): Update useCustomEnsuredControl types to reveal prevValue undefined #7285
mendrew merged 6 commits intomasterfrom
mendrew/fix/useCustomEnsuredControl/possible-undefined-pre-value

Conversation

@mendrew
Copy link
Copy Markdown
Contributor

@mendrew mendrew commented Aug 2, 2024

related: #7099

  • Unit-тесты - не удалось поймать этот случай в тестах

Описание

Выяснилось, что всё-таки, если передавать в свойство onChange useCustomEnsuredControl коллбэк nextValue c аргументом prevValue, то есть вероятность, что prevValue будет undefined.
Это видно, если исправить тип аргумента prevValue с any на V.

(nextValue: V | ((prevValue: any) => V)) => {

Дело в том, что preservedControlledValueRef, которое мы передаём в nextValue коллбэк как значение prevValue, может быть undefined, так как хранит предыдущее значение value, которое тоже может быть undefined.

} else if (onChangeProp) {
const resolvedValue = nextValue(preservedControlledValueRef.current);
onChangeProp(resolvedValue);
}

В теории (потому что в тестах не удалось повторить), если пользователь в 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, чтобы добавить в него новое значение.

    const toggleOption: ToggleOption<O> = React.useCallback(
    (nextValueProp: O | ChipOptionValue, isNewValue: boolean) => {
    setValue((prevValue) => {
    const isLikeObjectOption = isValueLikeChipOptionObject(nextValueProp);
    const resolvedOption = isLikeObjectOption
    ? getNewOptionData(nextValueProp.value, nextValueProp.label)
    : getNewOptionData(nextValueProp, typeof nextValueProp === 'string' ? nextValueProp : '');
    const nextValue = prevValue.filter((option: O) => resolvedOption.value !== option.value);
    if (isNewValue === true) {
    nextValue.push(
    isLikeObjectOption ? { ...nextValueProp, ...resolvedOption } : resolvedOption,
    );
    }
    return nextValue;
    });
    },
    [setValue, getNewOptionData],
    );

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

  4. Можно оставить как есть и добавить warning, как в реакте A component is changing an uncontrolled input to be controlled.
    В целом, это рабочий вариант, и ворнинг можно добавить к любому из остальных решений.
    Другое дело, что он будет менее понятен, так как хук будет использоваться внутри компонентов и сообщение будет абстрактным,и вроде как лучше бы не нагружать пользователя лишней информацией.
    Но всё же в этом есть смысл.

  • Добавили warning. Хотелось ещё дать ссылку на React warning, тогда надо пояснять, что в нашем случае речь не только про input, а как это сделать лаконично я не придумал.

@mendrew mendrew self-assigned this Aug 2, 2024
@github-actions github-actions bot added the ci:cherry-pick:patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label Aug 2, 2024
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 2, 2024

size-limit report 📦

Path Size
JS 378.24 KB (+0.03% 🔺)
JS (gzip) 114.69 KB (+0.04% 🔺)
JS (brotli) 94.09 KB (-0.06% 🔽)
JS import Div (tree shaking) 1.39 KB (0%)
CSS 305.67 KB (0%)
CSS (gzip) 39.22 KB (0%)
CSS (brotli) 31.48 KB (0%)

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 2, 2024

e2e tests

Playwright Report

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 2, 2024

👀 Docs deployed

Commit a17ec0d

@mendrew mendrew force-pushed the mendrew/fix/useCustomEnsuredControl/possible-undefined-pre-value branch from 7755cab to 8049364 Compare August 7, 2024 11:12
@codesandbox-ci
Copy link
Copy Markdown

codesandbox-ci bot commented Aug 7, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 81.25000% with 3 lines in your changes missing coverage. Please review.

Project coverage is 92.72%. Comparing base (08647eb) to head (a17ec0d).
Report is 68 commits behind head on master.

Files Patch % Lines
packages/vkui/src/hooks/useEnsuredControl.ts 80.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7285      +/-   ##
==========================================
+ Coverage   83.94%   92.72%   +8.77%     
==========================================
  Files         374      374              
  Lines       11108    11122      +14     
  Branches     3640     3651      +11     
==========================================
+ Hits         9325    10313     +988     
+ Misses       1783      809     -974     
Flag Coverage Δ
unittests 92.72% <81.25%> (+8.77%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mendrew mendrew marked this pull request as ready for review August 7, 2024 13:09
@mendrew mendrew requested a review from a team as a code owner August 7, 2024 13:09
@mendrew mendrew marked this pull request as draft August 7, 2024 13:41
mendrew added 4 commits August 7, 2024 16:47
prevState может быть undefined, и типы теперь это показывают.
Только не понятно как правильно этот undefined обрабатывать в
компонентах где используется useCustomEnsuredControl.
when the callback is passed as nextValue
@mendrew mendrew force-pushed the mendrew/fix/useCustomEnsuredControl/possible-undefined-pre-value branch from 1070b31 to 213cbd3 Compare August 7, 2024 13:48
@mendrew mendrew marked this pull request as ready for review August 7, 2024 14:10
inomdzhon
inomdzhon previously approved these changes Aug 14, 2024
Copy link
Copy Markdown
Contributor

@inomdzhon inomdzhon left a comment

Choose a reason for hiding this comment

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

👏👏👏

Спасибо за исчерпывающее описание 🙏

Co-authored-by: Victoria Zhizhonkova <[email protected]>
@mendrew mendrew added this to the v6.5.2 milestone Aug 22, 2024
@mendrew mendrew merged commit bb02ab8 into master Aug 22, 2024
@mendrew mendrew deleted the mendrew/fix/useCustomEnsuredControl/possible-undefined-pre-value branch August 22, 2024 07:17
vkcom-publisher pushed a commit that referenced this pull request Aug 22, 2024
… 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, а как это сделать лаконично я не придумал.
@vkcom-publisher
Copy link
Copy Markdown
Contributor

v6.5.2 🎉

andrey-medvedev-vk pushed a commit that referenced this pull request Sep 19, 2024
… 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, а как это сделать лаконично я не придумал.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci:cherry-pick:patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

5 participants