fix(useCustomEnsuredControl/useFloatingWithInteractions): Initialize preservedControlledValueRef with value #7099

mendrew · 2024-06-28T17:34:58Z

Описание

Мы получили репорт об ошибке от пользователей:

TypeError: Uncaught TypeError: Cannot read properties of undefined (reading 'shown')

shown в объекте используется только в useFloatingWithInteractions, который управляется с помощью хука useCustomEnsuredControl.
Единственный момент когда бы состояние могло быть undefined это момент когда вызывается callback

VKUI/packages/vkui/src/lib/floating/useFloatingWithInteractions/useFloatingWithInteractions.ts

Lines 93 to 102 in 406784a

    
           setShownLocalState((prevState) => { 
        
             if (prevState.shown !== nextShown || prevState.reason !== reason) { 
        
               return { 
        
                 shown: nextShown, 
        
                 reason, 
        
               }; 
        
             } 
        
             /* istanbul ignore next: страховка, если вдруг на момент вызова обновления состояния, оно уже будет актуальным */ 
        
             return prevState; 
        
           });

Тут мы вызываем setShownLocalState с функцией, у которой в аргументах может лежать предыдущее состояние стейта.
В этот момент у useCustomEnsuredControl, если компонент контролируемый и вызывает onChange с коллбэком, мы обращаемся к переменной preservedControlledValueRef.

VKUI/packages/vkui/src/hooks/useEnsuredControl.ts

Lines 70 to 73 in 406784a

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

Эта переменная может быть undefined только при инициализации, до тех пор пока в value не будет передано значение !== undefined.

VKUI/packages/vkui/src/hooks/useEnsuredControl.ts

Lines 49 to 53 in 7d4f104

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

Изменения

Инициализируем ref сразу же со значением value.

Воспроизведение

Мне никак не удалось довести компонент до состояния ошибки, ни локально, ни в тестовом окружении.
Возможно, что это вообще ложный след.
Возможно, что это также может происходить при изменении состояния компонента из неконтролируемого в контролируемое.

Потому что такая ошибка может возникнуть только если value был undefined, потом стал defined и был быстро вызван onChange , так быстро, что preservedControlledValueRef не успел обновится.

Возможно, что стектрейс, который привёл нас сюда не совсем верный. Его надо спрашивать в VK Teams.

We got an error from users about'shown' in useFloatingWithInteractions hook TypeError: Uncaught TypeError: Cannot read properties of undefined (reading 'shown') I wasn't able to catch this locally, or in test env, but to make sure preservedControlledValueRef can't be undefined if value is defind we give it a value.

codesandbox-ci · 2024-06-28T17:38:54Z

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.

github-actions · 2024-06-28T17:56:24Z

size-limit report 📦

Path	Size
JS	368.65 KB (+0.01% 🔺)
JS (gzip)	112.87 KB (0%)
JS (brotli)	92.91 KB (+0.05% 🔺)
JS import Div (tree shaking)	1.42 KB (0%)
CSS	289.84 KB (0%)
CSS (gzip)	37.66 KB (0%)
CSS (brotli)	30.44 KB (0%)

github-actions · 2024-06-28T17:59:42Z

e2e tests

Playwright Report

github-actions · 2024-06-28T17:59:55Z

👀 Docs deployed

✅ Styleguide
✅ Storybook

Commit 406784a

codecov · 2024-06-28T18:01:20Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.78%. Comparing base (7d4f104) to head (406784a).
Report is 2 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #7099   +/-   ##
=======================================
  Coverage   83.78%   83.78%           
=======================================
  Files         357      357           
  Lines       10676    10676           
  Branches     3551     3551           
=======================================
  Hits         8945     8945           
  Misses       1731     1731

Flag	Coverage Δ
unittests	`83.78% <100.00%> (ø)`

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.

inomdzhon

💅

We got an error from users about'shown' in useFloatingWithInteractions hook TypeError: Uncaught TypeError: Cannot read properties of undefined (reading 'shown') I wasn't able to catch this locally, or in test env, but to make sure preservedControlledValueRef can't be undefined if value is defind we give it a value.

We got an error from users about'shown' in useFloatingWithInteractions hook TypeError: Uncaught TypeError: Cannot read properties of undefined (reading 'shown') I wasn't able to catch this locally, or in test env, but to make sure preservedControlledValueRef can't be undefined if value is defind we give it a value. Co-authored-by: Andrey Medvedev <andr.medv.spb@gmail.com>

vkcom-publisher · 2024-07-08T16:33:15Z

✅ v6.2.1 🎉

… 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, а как это сделать лаконично я не придумал.

mendrew marked this pull request as ready for review June 28, 2024 17:44

mendrew requested a review from a team as a code owner June 28, 2024 17:44

mendrew self-assigned this Jun 28, 2024

mendrew added this to the v6.2.1 milestone Jun 28, 2024

mendrew added the type:bug label Jun 28, 2024

SevereCloud approved these changes Jul 1, 2024

View reviewed changes

inomdzhon approved these changes Jul 4, 2024

View reviewed changes

SevereCloud merged commit f49dd16 into master Jul 5, 2024
26 checks passed

SevereCloud deleted the mendrew/fix/useCustomEnsuredContro/initialize-preservedControlledValueRef branch July 5, 2024 12:24

inomdzhon added the patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label Jul 8, 2024

BlackySoul mentioned this pull request Jul 8, 2024

chirry-pick: 7099 #7154

Merged

mendrew mentioned this pull request Aug 2, 2024

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

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(useCustomEnsuredControl/useFloatingWithInteractions): Initialize preservedControlledValueRef with value #7099

fix(useCustomEnsuredControl/useFloatingWithInteractions): Initialize preservedControlledValueRef with value #7099

mendrew commented Jun 28, 2024

codesandbox-ci bot commented Jun 28, 2024

github-actions bot commented Jun 28, 2024

github-actions bot commented Jun 28, 2024

github-actions bot commented Jun 28, 2024

codecov bot commented Jun 28, 2024

inomdzhon left a comment

vkcom-publisher commented Jul 8, 2024

	setShownLocalState((prevState) => {
	if (prevState.shown !== nextShown \|\| prevState.reason !== reason) {
	return {
	shown: nextShown,
	reason,
	};
	}
	/* istanbul ignore next: страховка, если вдруг на момент вызова обновления состояния, оно уже будет актуальным */
	return prevState;
	});

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

	const preservedControlledValueRef = React.useRef<V>();

	useIsomorphicLayoutEffect(() => {
	preservedControlledValueRef.current = value;
	});

fix(useCustomEnsuredControl/useFloatingWithInteractions): Initialize preservedControlledValueRef with value #7099

fix(useCustomEnsuredControl/useFloatingWithInteractions): Initialize preservedControlledValueRef with value #7099

Conversation

mendrew commented Jun 28, 2024

Описание

Изменения

Воспроизведение

codesandbox-ci bot commented Jun 28, 2024

github-actions bot commented Jun 28, 2024

size-limit report 📦

github-actions bot commented Jun 28, 2024

e2e tests

github-actions bot commented Jun 28, 2024

👀 Docs deployed

codecov bot commented Jun 28, 2024

Codecov Report

inomdzhon left a comment

Choose a reason for hiding this comment

vkcom-publisher commented Jul 8, 2024