Что это такое?
Это код-ревью решений второго тестового задания. На видео отмечены удачные решения и указаны ошибки, а так же советы по их исправлению. В данной заметке отмечены общие проблемы и даны ссылки с "отметкой времени".
Общие проблемы
- Плохое Readme;
- Остаются eslint предупреждения, лишние
console.log(redux-логгер не в счет); - Иконка Web не вынесен вперед (невнимательное чтение задачи);
- Иконка Web выносится вперед в компоненте (а лучше бы в редьюсере, или экшене);
- Пароль не очищается, если запрос вернулся с ошибкой;
- Submit кнопка в форме логина доступна, если поля пустые (или одно из полей);
- Submit кнопка в форме логина не поддерживает нажатие Enter;
- Нет деления на компоненты/контейнеры (не относится к тем, кто делил по другим подходам);
- URL-адрес для запросов на сервер полностью передается (нет оформления повторяющейся части строки в константу);
- Ошибка/уведомление "неправильное имя пользователя/пароль" — не очищается;
- Ошибка "неправильное имя пользователя/пароль" — выводится константой с сервера;
- Текст ошибки "захардкожен" в коде. Нет обращения в словарик по константе с сервера;
- Не удален "старый код", то есть такой код, который нигде не используется;
- Нет блока catch у промисов, нет обработки ошибок, если сервер отвечает не
ok; - Компоненты размещены в node_modules;
- Отсутствуют или недостаточно подробно описаны Prop Types.
- Экшены и редьюсеры в куче в одном файле (или в одном все экшены, в другом все редьюсеры). Нет деления на "модули", то есть каждой сущности — свои экшены и свои редьюсеры;
Все решения с отметками по времени
Здесь указаны только ошибки, и не отмечены хорошие моменты, которых великое множество, поэтому крайне рекомендую посмотреть все подряд тем, кто считает себя "джуном" в разработке на react.
6m00s — Артур Донковцев Commit
7m40s — опечатка в названии функции
8m07s — асинхронные запросы не вынесены в экшены
10m07s — все экшены в одном файле. Нет деления на модули.
10m25s — вынос иконки (перебор данных) сделан в компоненте. Лучше в редьюсере или экшене.
12m28s — "прикольные" надписи. Лучше делать "нейтрально", чтобы затем подобные задания можно было сразу посылать работодателю.
13m05s — лишний экшен, указывающий что "загрузка" завершилась. То есть вместо трех экшенов: REQUEST / SUCCESS / STOP, можно уложиться в два: REQUEST / SUCCESS.
16m16s — Дмитрий Петров Commit
18m16s — использование var
18m34s — не вынесена часть URL адреса в константу
21m17s — плохое сообщение в коммите
22m15s — одинаковые названия экшенов.
24m16s — Кацура Владислав Commit
25m17s — (не ошибка) — данные приготовлены в редьюсере
27m38s — использование e.target, лучше e.currentTarget
28m20s — ==, а надо бы ===
28m33s — использование componentWillUnmount
29m00s (не ошибка) — рассуждение про "до серверную валидацию".
30m05s — код не отфморатирован (на любителя)
31m35s — использование "не универсального" обработчика, там где это уместно.
32m02s — Сергей Regies Linkas Commit
33m42s — нет экшена для прелоадера
34m30s — порядок методов в компоненте. (eslint плагин)
35m30s — не существующий PropTypes
35m57s — Кононов Виталий Commit
39m45s — не делаем то, что не интересно
40m31s — Евгений Санжиев Commit
41m20s (не ошибка) — словарик для работы с ошибками
42m46s — Виталий Набережный Commit
42m54s — не убраны тестовые данные
44m50s — Вениамин Трепачко Commit
Ачивка: очень классный дизайн.
47m42s — redux версия не полноценная.
47m57s — Ingvarr6 (Игорь) Commit
48m21s — нет 404 роута
51m30s — не очищается ошибка
54m48s — Роман Палесика Commit
55m30s — не хватает экшенов на загрузку/ошибку
56m49s — использование side-effects в редьюсере
58m10s — (не ошибка) вынос иконки web с помощью css (sick!)
59m15s — использование callback'a в setState, что приводит к лишней перерисовке. Лучше валидировать прямо в рендере.
61m01s — неуместное использование else if
62m13s — dsfcv d (boortcore) Commit
63m15s Константин Липский Commit
65m11s — в экшен передается URL целиком, лучше просто id передавать в данном варианте.
67m50s — сложное условие в shouldComponentUpdate, можно проще (сразу проверить на props.data и все)
69m32s — e.preventDefault не первый в обработчике
71m50s — Ахметанов Альберт Commit
72m20s — компоненты в node_modules
73m15s — дублирование обращений к переменным
76m04s — privateRoute не вынесен в отдельный компонент
76m33s — сложный код для перемещения иконки web
76m56s — избыточное свойство loaded
77m35s — Аладьин Александр Commit
80m33s — ошибка не вынесена в словарик
81m43s — избыточное использование withRouter
83m04s — Dmitrii Shapovalenko Commit
84m58s — избыточное деление экшенов
85m55s — ошибка в названии lifecycle-метода
87m15s — семантически неверное использование тэга article
90m46s — лишний вызов метода массива
Комментарии (25)
Lexicon
04.06.2018 22:51+1Скажу только об "общем" списке ошибок.
Не поймите неправильно, но на мой взгляд, практически каждая из ошибок — ошибка невнимательности или лени. И я более, чем убежден, что часть вины в задании(двух), полностью описанном текстом(ни макетов, ни примера) и структурированом таким образом, что респонднту определенно придется бегать по всему тексту глазами. Некоторых моментов (например последний пункт), я вовсе не нашел бегло разглядывая задание.
И хотя ошибки невнимательности определенно важны при собеседовании, они не несут никакой реальной информации, помимо того, что респондент не готов тратить время, требуемое для выполнения этого задания(двух).
P.S. Надеюсь не обидетесь, в прошлом ваши статьи не читал, поэтому попросил бы разъяснить значение термина "junior-react" из заголовка.
maxfarseer Автор
05.06.2018 09:30Спасибо за развернутый комментарий. По макету/примеру хорошее замечание, возможно, будут добавлены в будущем.
Про детали и лень — хороший пример. Если делаешь — делай хорошо. Фронтенд, это не только про «вау как здорово и классно, и платят много», это так же про не любимые «доработки/допиливания» мелких деталей, особенно возня с формами и ошибками как раз. Мы на видео этого коснулись. На трансляции некоторые ребята ответили — да, было лень. С ленью надо работать. Заодно они увидели реальные примеры того, что им могут отправить на доработку после code-review. Поэтому, я считаю, что это «несет реальную информацию».
p.s. хабр взрастил какой-то пласт обидчивых авторов и комментаторов?) Конструктивная критика всегда хорошо, никаких обид. Для меня junior react — начинающий разработчик (фронтендер), который пишет на стэке react. Это отчасти и начинающий javascript разработчик, или не начинающий js разрабочтик, но начинающий в react, или наоборот ничего не знающий в javascript, но так же тот, кто начал свой путь с react.
Frimko
05.06.2018 07:52я 1 не понимаю откуда у этой статьи растут ноги?
maxfarseer Автор
05.06.2018 09:33Не понял вопроса.
Frimko
05.06.2018 10:57я к тому, где вообще ссылка на предыдущую статью этого вебинара? Наткнулся на этот пост чисто случайно. Без ссылки на анонс этого теста и теста первого, пост кажется каким-то ущербным.
maxfarseer Автор
05.06.2018 11:50Вас понял. Я не делал анонса на хабре, так как не знаю как это здесь подать. У меня платного аккаунта нет, а анонс больше подходит под рекламу. В противовес этому, в разборе уже без ссылок (только на github), без призывов к переходу в паблики, разобраны ошибки, которые в отрыве от всей движухи в целом тянут на отдельную заметку, полезную для начинающих.
p.s. надо подумать, как можно сделать анонс и не разгневать публику. Аккаунт с карточкой подписи, в которой можно указать vk/telegram и тд на 3 месяца стоит дорого (14 тысяч), и тоже не особо позволяет постить ссылки в самой статье, а корпоративный аккаунт стоит и того дороже (было 70).Frimko
05.06.2018 13:13ну так что тут думать. Описываем историю своего начинания, завязываем сюжетную линию, делаем развязку и как итог снизу в виде выводов этот пост.
Druu
05.06.2018 11:19Иконка Web выносится вперед в компоненте (а лучше бы в редьюсере, или экшене);
А если будет два компонента, в одном из которых нужно иконку отрисовать первой, а в другом — последней?
maxfarseer Автор
05.06.2018 11:53Хороший вопрос. Вижу два варианта:
1) диспатчнуть два экшена (например, SET_ICONS_FOR_COMP_A и SET_ICONS_FOR_COMP_B)
2) делать это в компоненте
В зависимости от частоты перерисовок компонента и каких-то еще условиий задачи, можно выбрать то, что больше подойдет. Мне пока импонирует вариант 1.
Так или иначе, в задаче хотелось показать, что если вам с бэкэнда пришли не угодные данные, то на мой взгляд, лучше их «перевернуть» в редьюсере.
Druu
05.06.2018 12:05+11) диспатчнуть два экшена (например, SET_ICONS_FOR_COMP_A и SET_ICONS_FOR_COMP_B)
Тогда либо вы храните одну копию данных и последовательное выполнение второго экшена "сломает" данные предыдущего (что ломает логику приложения, если оба компонента находятся на странице одновременно либо отображаются друг за другом без перезагрузки данных), либо вы храните две копии и это нарушает SSOT.
Так или иначе, в задаче хотелось показать, что если вам с бэкэнда пришли не угодные данные, то на мой взгляд, лучше их «перевернуть» в редьюсере.
Все зависит от того, что считать "неугодными".
Система стор-экшоны-редьюсер — это m из mvc, модель ничего не должна знать о свойствах вида (о том, что вашему виду нужно вывести какой-это элемент первым). Это целиком и полностью ответственность вида, то есть компонента. В модели данные могут вообще находиться в сильно другой форме, чем потом выводится пользователю.
Если же есть какие-то ограничения на данные, которые следуют непосредственно из предметной области — тогда да, следует провести преобразование в редьюсере/экшоне.
Xu4
05.06.2018 13:02Отсутствуют или недостаточно подробно описаны Prop Types.
Я всегда считал, что PropTypes — это фишка только для development-версии. Когда это ещё было встроенным функционалом React.js, это игнорировалось в продакшене, потому что даёт снижение производительности приложения, (Насколько я понимаю, отдельный пакет prop-types, который появился после версии 15.5, тоже работает только в режиме development.)
По сути, PropTypes отрабатывает только в режиме разработки. Это значит, что если в продакшене для какого-то свойства был передан неподходящий параметр, продакшн от этого не будет защищён, потому что вся ответственность за типы была передана в PropTypes, который в продакшене игнорируется. Когда навязывается использование PropTypes, у разработчиков складывается ложное впечатление, что они предусмотрели больше, чем это есть на самом деле, что ведёт к снижению качества продукта.
По сути, нет смысла тратить время на PropTypes. Лучше его потратить на то, чтобы компонент самостоятельно мог выдавать ошибки или мог принудительно генерировать дефолтные значения для props, если ничего не было передано (или конвертировать входные значения в подходщий формат, если они имеют неправильный). Чтобы PropTypes мог это делать, нужно либо использовать в продакшене development-версию React, либо модифицировать его, чтобы PropTypes мог работать в продакшене.
Основная моя критика в этом:
- В режиме production PropTypes не защищает от неправильных входных данных;
- В режиме production PropTypes нарушает ещё один ваш критерий оценки: «Не удален „старый код“, то есть такой код, который нигде не используется»
maxfarseer Автор
05.06.2018 13:07Спасибо за мнение, не совсем согласен.
PropTypes — это некий конспект того, что должно приходить в компонент и в каком виде. В процессе разработки и поддержки — это очень удобно. Конечно, игнорирование предупреждений или внезапное изменение чего-то от бэкэнда, приведет к проблемам в production версии.
чтобы компонент самостоятельно мог выдавать ошибки или мог принудительно генерировать дефолтные значения для props, если ничего не было передано (или конвертировать входные значения в подходщий формат, если они имеют неправильный)
Здесь +1, но не согласен, что делать это «лучше, вместо описывания PT».
p.s. вместе с flow, propTypes становятся еще более удобными в режиме разработки и поддержки.Xu4
05.06.2018 13:12p.s. вместе с flow, propTypes становятся еще более удобными в режиме разработки и поддержки.
В режиме поддержки PropTypes не становятся более удобными, потому что поддержка обычно осуществляется для пользователей, а пользователи — это те, кто использует production-версию, и в режиме поддержки приходится обычно работать с кодом и логами оттуда, где PropTypes не работают.faiwer
05.06.2018 13:48Если у вас и правда часто возникает такая необходимость,
то пишите тестыто сделайте так, чтобы PropTypes работали и в prod-версии. К примеруnpm:check-prop-types.
Xu4
05.06.2018 13:19Удобно в качестве конспекта — да. Но это удобство в ущерб стабильности. Грубо говоря, разработчику будет удобно разрабатывать код 10 человеко-часов, но 10 000 000 человеко-часов код будет работать нестабильно на production-сервере.
faiwer
05.06.2018 13:47+1Лучше его потратить на то, чтобы компонент самостоятельно мог выдавать ошибки или мог принудительно генерировать дефолтные значения для props, если ничего не было передано
Дык это будет не бесплатно. Любой runtime-код это runtime-код. Вне зависимости от того, библиотечный он или нет. Проверок PropTypes нет в prod-коде именно из-за этого. Также как и никто не прогоняет тесты на машинах клиентов, их прогоняют на build-серверах, на локальных машинах разработчиков. DEV-инструменты это DEV-инструменты. Также как и eslint…
В общем ваша критика непонятна. PropTypes очень сильно выручают в случае, если вы не используете flow или TypeScript. А вот насколько они нужны, если всё таки статическая типизация используется я судить не берусь.
maxfarseer Автор
05.06.2018 13:58Если типизация есть, тоже удобно. PT просто сразу описываются на flow синтаксисе (плагин)
Frimko
05.06.2018 14:07а мне кажется, что джунам нужны четкие правила, что делать плохо и что хорошо. Лишь со временем человек понимает, что — «Тут можно сделать по правилам, а тут их лучше проигнорировать». Это очень хорошо довели в одном докладе на holyJS youtu.be/5IjUVUlkpvQ?t=17478
так что данная статья больше подходит для самих джунов и тех, кто их будет нанимать. Для мидла и выше подобное уже не котируется, сказывается опыт в проектах и способы решения проблем.
Demidenko139
«Ошибка „неправильное имя пользователя/пароль“ — выводится константой с сервера;»
Если я правильно понял, то текст ошибки приходит с сервера. Я вообще не React-разработчик, но почему так нельзя? Многие backend-фреймворки умеют валидировать данные и выводить текст ошибок. Какой смысл его (текст ошибки) переопределять на фронтенде?
maxfarseer Автор
Зависит от бэкэнда. Если бэк сразу выдает «читаемую» ошибку — то можно сразу ее и выводить. Если нет — то нужно «переопределить». В задании, бэкэнд не умел выдавать красивую ошибку, а присылал: wrong_email_or_password
xadd
Имхо, вся обязанность бэкенда — это отдать код ошибки и все полезные с ней данные, а как отобразить ее пользователю и на каком языке — это уже ответственность клиентской части.
maxfarseer Автор
мне тоже импонирует такой подход
Sultansoy
Мне сей подход тоже импонирует, но, в каком виде отдавать коды ошибок? Классические http? Или что-то свое кастомное? Не приведет ли кастомное к ошибкам. Например сервер возвращает строку «wrong_email_or_password», а на клиенте при проверке на равенство я потерял букву. Насчет локализации, что фронтенд выбирает, на каком языке, это получается у фронта сразу вшито несколько языковых пакетов?
Как вам такой дизайн. Бэкэнд выдает ошибку, а фронт обращается к бэку еще раз с ошибкой и нужным языком, чтобы получить текст ошибки?
SirrioN
При локализации, мне кажется, текущий код языка пользователя (в мультиязычном приложении) должен отправляться на сервер с каждым запросом (куки/заголовок). Соответственно сервак должен отдавать уже локализированый ответ.
А плодить кучу запросов, мне кажется совсем не верным решением.
xadd
Кастомные. Тут можно использовать строковые константы. Если бэкенд на ноде, то можно использовать общий код, иначе держать константы в синхронизированном виде для обоих частей.