appbooster / eslint-config-react

Appbooster's ESLint config for applications with react, following our styleguide
MIT License
1 stars 0 forks source link

Add jsx-no-bind rule #10

Closed m1neral closed 5 years ago

m1neral commented 5 years ago

Запрещаем использовать стрелочные функции в пропсах, т.к. является антипаттерном и ухудшает производительность:

https://mobx.js.org/best/react-performance.html#bind-functions-early https://reactjs.org/docs/handling-events.html

The problem with this syntax is that a different callback is created each time the LoggingButton renders. In most cases, this is fine. However, if this callback is passed as a prop to lower components, those components might do an extra re-rendering. We generally recommend binding in the constructor or using the class fields syntax, to avoid this sort of performance problem.

KELiON commented 5 years ago

🤔вообще выглядит разумно, но иногда так и хочется (а иногда так даже и удобнее читать код) хочется написать через propName={() => ...}. Думаю можно это вмёржить, а если будет место, где это сделано умышленно — писать eslint-ignore. @Komalov нет возражений?

rtt63 commented 5 years ago

@KELiON есть

               <Scene
                  key="onboarding-welcome"
                  component={() => <WelcomeScreen onPressButton={onTapOkOnWelcomeScreen} />}
                  type="replace"
                  panHandlers={null}
                />

Это из нашего App.js, Pure-компонента, который не принимает пропсов и не содержит стейта. Здесь функция нам никак не портит оптимизацию, потому что ререндеров не происходит. Однако с запретом на функцию нам придется ссылаться на компонент и пропсы неявно добавлять самой сцене, что мы уже давно нашли неудобным. Вместе с тем у нас есть конструкции типа

                <Stack
                  key="offers"
                  title={I18n.t('screens.offers')}
         THIS ->  icon={({ tintColor }) => <Icon name="check" size={24} color={tintColor} />}
                  type="replace"
                  initial={true}
                >

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

Мы очень часто будем писать eslint-ignore.

Предлагаю сделать это правило варнингом, а не ошибкой (как с console.log)

m1neral commented 5 years ago

Мы очень часто будем писать eslint-ignore. Предлагаю сделать это правило варнингом, а не ошибкой (как с console.log)

Встречно предлагаю оставить этот конфиг с error, и вам, у себя в проекте переопределить глобально на ворнинги (или вообще отключить, чтобы не ворнинги не сыпались) - https://github.com/appbooster/appbonus-react-native/blob/master/.eslintrc.yml#L7

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

rtt63 commented 5 years ago

@m1neral как вариант. В целом уже был момент, что я кому-то писал про это в ревью. Конечно это надо отлавливать на стадии написания кода, а не на ревью

KELiON commented 5 years ago

@Komalov у нас есть один файл, где это всё уместно — это App.js (ну с недавних пор ещё всё в папке routes). Предлагаю игнорить это правило только в этих файлах, а в остальных придерживаться