Skip to content
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

fix(Hint,Tooltip,Tab): process children with imperative handles correctly #2939

Merged
merged 18 commits into from
Sep 6, 2022

Conversation

lossir
Copy link
Member

@lossir lossir commented Jul 1, 2022

Проблема

Использование хука useImperativeHandle может развалить приложение.
Обнаружил во время ревью фикса другой проблемы связанной с getRootNode #2930 (comment)

Там компонент FileUploader начал валиться с такой ошибкой:

Error: Argument appears to not be a ReactComponent. Keys: focus,blur,reset
    at findHostInstanceWithWarning
-   at findDOMNode
    ...

Она возникает здесь:

rootNode = findDOMNode(instance);

А причина тут:

useImperativeHandle(ref, () => ({ focus, blur, reset }), [ref]);

findDOMNode пытается вытащить ноду из простого объекта и разваливается.

Т.е. если пользователь также будет использовать в своём компоненте хук useImperativeHandle и обернёт его, например, в Тултип, то его приложение развалится.

Решение

try/catch

В качестве фикса я использовал try/catch которые позволяет приложению просто не крашиться.
Текст ошибки в консоле содержит ссылку на workaround

-[getRootNode]: using findDOMNode threw an exception
-See https://github.com/skbkontur/retail-ui/blob/master/packages/react-ui/README.md#strictmode

Тултип и ему подобные контролы всё равно не будут работать, т.к. не смогут достать дом-ноду.

пример 1

Но можно немного доработать объект возвращаемый в useImperativeHandle, и всё заведётся.

const coolRef = useRef<any>();
useImperativeHandle(ref, () => ({
  ...
  getRootNode: () => coolRef.current
}));
return <span ref={coolRef} />;

пример 2

Можно ещё просто обернуть пользовательский контрол в useImperativeHandle в span/div и тогда findDOMNode нормально отработает.

Ссылки

Воспроизведение бага можно посмотреть в песочнице. Там же реализован workaround:
https://codesandbox.io/s/getrootnode-useimperativehandle-workaround-qrklrd?file=/src/App.tsx

По тестам Реакта видно, что findDOMNode так и должен себя вести:
https://github.com/facebook/react/blob/main/packages/react-dom/src/__tests__/findDOMNode-test.js#L66-L70

IF-577

Чек-лист перед запросом ревью

  1. Добавлены тесты на все изменения
    ✅ unit-тесты для логики
    ⬜ скриншоты для верстки и кросс-браузерности
    ⬜ нерелевантно

  2. Добавлена (обновлена) документация
    ⬜ styleguidist для пропов и примеров использования компонентов
    ⬜ jsdoc для утилит и хелперов
    ✅ комментарии для неочевидных мест в коде
    ✅ прочие инструкции (README.md, contributing.md и др.)
    ⬜ нерелевантно

  3. Изменения корректно типизированы
    ✅ без использования any (см. PR 2856)
    ⬜ нерелевантно

  4. Прочее
    ✅ все тесты и линтеры на CI проходят
    ✅ в коде нет лишних изменений
    ✅ заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)

@lossir lossir requested review from zhzz and JackUait July 1, 2022 12:07
@lossir lossir marked this pull request as ready for review July 1, 2022 12:07
Copy link
Contributor

@JackUait JackUait left a comment

Choose a reason for hiding this comment

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

Да, похоже, что с классовыми компонентами (на нашей стороне) можно предложить только обходное решение. С функциональными компонентами в теории можно было бы попробовать использовать способ, который мы использовали в withClassWrapper

packages/react-ui/lib/rootNode/getRootNode.ts Outdated Show resolved Hide resolved
@lossir lossir requested review from zhzz and JackUait July 14, 2022 06:16
@zhzz zhzz marked this pull request as ready for review September 5, 2022 06:18
@zhzz zhzz force-pushed the fix-getRootNode-findDOMNode-useImperativeHandle branch from 29f0a1c to f46a21f Compare September 5, 2022 07:32
@zhzz
Copy link
Member

zhzz commented Sep 5, 2022

@lossir @JackUait не могу перезапросить вашего ревью. ПР готов, прошу глянуть.

@lossir
Copy link
Member Author

lossir commented Sep 6, 2022

👍

@zhzz zhzz changed the title chore: fix case where findDOMNode crashes app fix(Hint,Tooltip,Tab): process children with imperative handles correctly Sep 6, 2022
@zhzz zhzz merged commit 7958543 into master Sep 6, 2022
@zhzz zhzz deleted the fix-getRootNode-findDOMNode-useImperativeHandle branch September 6, 2022 06:35
JackUait added a commit that referenced this pull request Sep 15, 2022
zhzz pushed a commit that referenced this pull request Oct 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants