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

[react-ui] Использовать деструктуризацию только когда она необходима IF-634 #2642

Open
JackUait opened this issue Nov 14, 2021 · 0 comments

Comments

@JackUait
Copy link
Contributor

JackUait commented Nov 14, 2021

Все примеры приведены на основе отрефактаренного контрола Link из #2635.

Сейчас в коде деструктуризация используется повсеместно, считаю, что деструктуризация должна быть использована только там, где она необходима.

На это есть несколько причин:

  1. Ухудшается читаемость кода.
    Утверждение №1: Если пропы используются только в одном компоненте, нет смысла хранить их в переменной и затем деструктурировать в компонент. Это только заставит разработчика раз за разом терять контекст.

    Как это реализовано сейчас (пример №1):
    В компоненте создаётся объект со всеми пропами, которые затем просто деструктурируются в элемент ссылки.

    const linkProps = {
        className: cx({
          [styles.root(theme)]: true,
          [styles.button(theme)]: !!_button,
          [styles.buttonOpened()]: !!_buttonOpened,
          [styles.useDefault(theme)]: use === 'default',
          [styles.useSuccess(theme)]: use === 'success',
          [styles.useDanger(theme)]: use === 'danger',
          [styles.useGrayed(theme)]: use === 'grayed',
          [styles.useGrayedFocus(theme)]: use === 'grayed' && focused,
          [styles.focus(theme)]: focused,
          [styles.disabled(theme)]: !!disabled || !!loading,
        }),
        href,
        rel,
        onClick: handleClick,
        onFocus: handleFocus,
        onBlur: handleBlur,
        tabIndex: disabled || loading ? -1 : tabIndex,
      };
    
      return <a {...linkProps}>...</a>

    Основная проблема в рамках ухудшения читаемости кода - создание дополнительной, ненужной абстракции, которая просто хранит все пропы нужные компоненту, не принося практической пользы. Более того, эта абстракция использует менее очевидный, сокращённый синтаксис для задания пропов href и rel.

    Приведённый пример достаточно простой и тут очевидно какие пропы попадают в компонент. Прежде чем читать дальше, предлагаю разобраться с тем, какие пропы попадают во вложенные компоненты в контроле FxInput и найти проп, который может привести к потенциальному багу и проблеме с производительностью одновременно. Ответ в конце раздела.

    Как это следует реализовать (пример №2):

          <a
            href={href}
            rel={rel}
            onClick={handleClick}
            onFocus={handleFocus}
            onBlur={handleBlur}
            tabIndex={disabled || loading ? -1 : tabIndex}
            className={cx({
              [styles.root(theme)]: true,
              [styles.button(theme)]: !!_button,
              [styles.buttonOpened()]: !!_buttonOpened,
              [styles.useDefault(theme)]: use === 'default',
              [styles.useSuccess(theme)]: use === 'success',
              [styles.useDanger(theme)]: use === 'danger',
              [styles.useGrayed(theme)]: use === 'grayed',
              [styles.useGrayedFocus(theme)]: use === 'grayed' && focused,
              [styles.focus(theme)]: focused,
              [styles.disabled(theme)]: !!disabled || !!loading,
            })}
          >
            ...
          </a>

    Изменения в коде произошли минимальные, но были решены сразу три проблемы:

    1. Удалён лишний уровень абстракции.
    2. Теперь чётко видно в какой элемент приходят пропы.
    3. Нет неявных определений href и rel.

    Примечание: сильно разросшиеся объявления className (15-20+ вхождений) можно выносить в отдельную сущность.

    Сравните один и тот же сценарий поиска пропа в компоненте до и после рефакторинга:
    До: найти нужный проп -> понять что он хранится в объекте -> проскроллить до объявления объекта -> посмотреть в какой компонент передаётся этот объект -> вернуться к объекту -> проскроллить до пропа => контекст потерян.

    До рефакторинга на примере
    search-prop-before.mov

    После: найти нужный проп -> посмотреть в какой компонент передаётся этот проп => остаёмся в контексте.

    После рефакторинга на примере
    search-prop-after.mov
    Ответ на вопрос про FxInput

    Правильный ответ: hideTrailingZeros.

    Этот проп передаётся как в контрол Input, так и в контрол CurrencyInput, но контрол Input не знает о существовании hideTrailingZeros, что потенциально может привести к багу, например, если будут ужесточены правила в tsconfig.

    По этой же причине может появиться потенциальная проблема с производительностью: проп hideTrailingZeros может быть передан в FxInput, когда FxInput рендерится как контрол Input, заставляя контрол Input ререндериться при изменении пропа hideTrailingZeros.
    Эти проблемы достаточно сложно отследить из-за использования деструктуризации.

    Вывод: не используйте объект для хранения пропов, если можно обойтись без него.

    Утверждение №2: деструктуризация пропов затрудняет чтение кода и визуально раздувает компонент.

    Обращение к деструктурированным пропам затрудняет чтение кода. Чтобы понять something это функция/переменная доступная внутри файла или проп приходящий снаружи, нужно понять контекст.

    Поиск контекста с деструктурированными пропами: поднимаемся до объявления компонента -> смотрим на пропы -> если там есть нужный нам проп -> возвращаемся к задаче -> если нужного пропа там нет -> продолжаем искать дополнительную деструктуризацию в компоненте -> возвращаемся к задаче => в обоих случаях теряем контекст.

    Поиск контекста с объектом props: обращение выглядит как props.something или rest.something? -> это проп => остаёмся в контексте.

    Плохое и хорошее объявление пропов (пример №3):

    Плохо 👎
    const {
          disabled = false,
          href = '',
          use = 'default',
          icon,
          loading,
          _button,
          _buttonOpened,
          rel: relOrigin,
          tabIndex,
          children,
          onClick,
          ...rest
    } = props;
    
    
    Офигенно 👍
    const {
          disabled = false,
          href = '',
          use = 'default',
      	...rest
    } = props;

    Используйте деструктуризацию пропов только в том случае, если пропам нужно задать значение по умолчанию.
    Примечание: получить дефолтные значения пропов disabled, href и use с помощью конструкции props.x не получится, так как обращение будет идти к изначальному объекту props без заданных значений по умолчанию. К этим трём пропам придётся обращаться напрямую.

    Более правильным подходом с точки зрения консистентности обращения к пропам, было бы задание значений по умолчанию через defaultProps.
    Задание пропов через defaultProps позволяет обращаться ко всем пропам (в том числе к пропам с заданными значениями по умолчанию) через синтаксис props.x, но к сожалению в одном из будущих релизов, команда React удалит defaultProps из API функциональных компонентов. Помимо этого, конструкция defaultProps работает медленнее, чем задание значений по умолчанию, через деструктуризацию, более подробно об этом в утверждении №4 раздела перформанс.

    В примере Офигенно 👍 было пропущено переименование rel в relOrigin, которое присутствует в примере Плохо 👎. Это было сделано намерено. Так как это объявление можно опустить, потому что обращение к rel теперь происходит с помощью rest.rel, следовательно запись let rel = rest.rel (пример из контрола Link) вполне легальна. Более подробно о том, почему запись rest.rel лучше в первой аксиоме раздела перформанс.

    Выводы:

    1. Старайтесь
    2. Используйте деструктуризацию только для пропов, которым необходимо значение по умолчанию.
    3. Обращайтесь к пропам без значения по умолчанию, используя синтаксис props.something или rest.something.

  1. Перформанс.
    Прежде чем продолжить, нужно определить три аксиомы:

    1. Деструктуризация - не магия, а синтаксический сахар над Object.assign/Array.Prototype.concat (в зависимости от контекста). Более того, эта "не магия" зачастую выполняется за линейное время O(n) (а иногда и за квадратичное время O(n^2)), где n - это количество полей объекто-подобной сущности, которые нужно вытащить.
    // Нетривиальная операция по вычленению `prop1`, `prop2` и `prop3` из `props` и помещение их в `rest`.
    // С квадратичной сложностью выполнения `O(n^2)`
    const {prop1, prop2, prop3, ...rest} = props;
    1. Обращение к полю объекта выполняется за константное время O(1).
    const {value1, value2} = result; 
    
    // Всё равно что:
    const value1 = result.value1;
    const value2 = result.value2;

    Поэтому данный тип деструктуризации (без использования Rest) будет иметь линейную сложность O(n).

    1. Функциональные компоненты - это буквально функция render() из классовых компонентов. При каждом ререндере функциональный компонент, подобно функции render() будут создан с нуля.

    Теоремы:

    1. Как было описано в утверждении №1: объект с пропами LinkProps не несёт в себе никакой функциональной нагрузки и только затрудняет читаемость кода. Со стороны перформанса этот объект также занимает лишнее место в памяти, а согласно третьей аксиоме объект LinkProps будет пересоздан при каждом ререндере, создавая проблему с памятью. Эту проблему можно избежать, следуя действиям описанным в утверждении №1.
    2. Помимо проблемы с памятью из первой теоремы, мы также столкнёмся с проблемой с производительностью, так как при каждом ререндере мы мало того, что будем создавать лишний объект, так ещё и будем вызывать лишний Object.assign, для того, чтобы деструктурировать объект LinkProps в элемент ссылки. Эту проблему можно избежать, следуя действиям описанным в утверждении №1.
    3. Если у компонента нет пропов со значением по умолчанию, нужно использовать обращение props.something, о чём было сказано в выводах утверждения №2. Причина в том, что ненужная деструктуризация пропов, согласно первой и третьей аксиомам будет замедлять код. И согласно второй аксиоме, обращение к props.something будет занимать константное время. Следовательно, код будет работать быстрее.
    4. Неочевидный плюс перехода с классовых компонентов на функциональные - отказ от defaultProps. Конструкция defaultProps вызывается при каждом ререндере компонента (также как и Object.assign при деструктуризации пропов), но работает значительно медленнее, по сравнению с заданием значения по умолчанию, используя синтаксис ES6.
    5. Порядок значений при использовании оператора Spread и TypeScript имеет значение.
    // TS:
    res = {a: 1, ...obj};
    
    // Компилируется в ES5:
    res = __assign({a: 1}, obj);

    При таком расположении значений компилиция происходит правильно.

    Если поменять значения местами...

    // TS:
    res = {...obj, a: 1};
    
    // Компилируется в ES5:
    res = __assign(__assign({}, obj), {a: 1});
    
    // Хотелось бы:
    res = __assign({}, obj, {a: 1});

    ...мы получим лишний, вложенный вызов __assign.

    При этом, это не баг TypeScript'а. Более подробно причины этого поведения описаны в статье в разделе TypeScript.


Итого: следуя трём правилам из выводов раздела ухудшается читаемость кода и правилу из пятого утверждения раздела перформанс можно значительно увеличить производительность кода.

Отрефакторенный согласно всем правилам контрол Link будет выглядеть так:

const Link = forwardRef<HTMLAnchorElement, React.PropsWithChildren<LinkProps>>((props, ref) => {
  const { disabled = false, href = '', use = 'default', ...rest } = props;

  const theme = useContext(ThemeContext);

  const [focusedByTab, setFocusedByTab] = useState(false);
  const focused = !disabled && focusedByTab;

  let iconElement: React.ReactNode = null;
  if (rest.icon) {
    iconElement = (
      <span className={styles.icon(theme)}>
        {rest.loading ? <Spinner caption={null} dimmed type="mini" /> : rest.icon}
      </span>
    );
  }

  let arrow: React.ReactNode = null;
  if (rest._button) {
    arrow = <span className={styles.arrow()} />;
  }

  let rel = rest.rel;
  if (typeof rel === 'undefined' && href) {
    rel = `noopener${isExternalLink(href) ? ' noreferrer' : ''}`;
  }

  const handleClick = (event: React.MouseEvent<HTMLAnchorElement>) => {
    if (!href) {
      event.preventDefault();
    }
    rest.onClick?.(event);
  };

  const handleFocus = () => {
    if (!disabled) {
      // focus event fires before keyDown eventlistener
      // so we should check tabPressed in async way
      requestAnimationFrame(() => {
        if (keyListener.isTabPressed) {
          setFocusedByTab(true);
        }
      });
    }
  };

  const handleBlur = () => {
    setFocusedByTab(false);
  };

  return (
    <CommonWrapper {...props}>
      <a
        {...rest}
        ref={ref}
        href={href}
        rel={rel}
        onClick={handleClick}
        onFocus={handleFocus}
        onBlur={handleBlur}
        tabIndex={disabled || rest.loading ? -1 : rest.tabIndex}
        className={cx({
          [styles.root(theme)]: true,
          [styles.button(theme)]: !!rest._button,
          [styles.buttonOpened()]: !!rest._buttonOpened,
          [styles.useDefault(theme)]: use === 'default',
          [styles.useSuccess(theme)]: use === 'success',
          [styles.useDanger(theme)]: use === 'danger',
          [styles.useGrayed(theme)]: use === 'grayed',
          [styles.useGrayedFocus(theme)]: use === 'grayed' && focused,
          [styles.focus(theme)]: focused,
          [styles.disabled(theme)]: !!disabled || !!rest.loading,
        })}
      >
        {iconElement}
        {rest.children}
        {arrow}
      </a>
    </CommonWrapper>
  );
});

Итоги рефакторинга:

  1. Улучшилась читаемость кода.
  2. Увеличилась производительность кода.
  3. Сократился размер кода и дублирующихся значений.
@dzekh dzekh changed the title Использовать деструктуризацию только когда она необходима [react-ui] Использовать деструктуризацию только когда она необходима IF-634 Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant