Sprint-4/Step-2 #5
Conversation
There was a problem hiding this comment.
Доброго времени суток! ☀️ 🌙
Вы проделали очень хорошую работу по типизации вашего проекта, практически везде прописаны корректные типы, учтены утилитарные функции, созданы кастомные типы для расширения типов, так держать 👍
Тем не менее, для успешной сдачи работы вам необходимо внимательно ознакомиться с комментариями, оставленными в рамках ревью и провести необходимые изменения, обратив особое внимание на следующие моменты:
- Следует улучшить типизацию для функции
checkResponse, чтобы воспользоваться полностью возможностями TS для проверки качества и корректности вашего кода (подробнее в комментариях к файлу); - Следует разрешить ситуацию с типизацией рефов (множественных рефов) согласно предложенным комментариям;
- (Не относится к Redux файлам сейчас): Использование типа
anyв большинстве случае нецелесообразно, т.к. этот подход не позволяет анализатору кода своевременно проверять его на ошибки, которые могут быть выявлены на этапе сборки проекта и усложняет вашу работу с кодом, т.к. значения будут неочевидными (см. https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html#any) - (Не относится к Redux файлам сейчас): В проекте присутствуют отключения линтера с помощью директивы
// @ts-ignore. Такой подход считается плохой практикой и должен использоваться только в крайних случаях, когда по-другому не получается объяснить TS, как ему понять данный код корректно.
Также, в рамках ревью были предложены улучшения вашего кода, которое могут повысить его качество, исправить возможные проблемы в будущем и углубить ваше значение по курсу.
Если у вас возникнут вопросы относительно комментариев к коду - пишите их в соответствующем треде или передайте их через вашего куратора. Обязательно найдём самое лучшее решение :)
Успешного вам рефакторинга, хорошего настроения и отличного дня! 🐈
| ingredients.reduce((acc, cur) => { | ||
| if (cur.ingredient.price) { | ||
| return acc + cur.ingredient.price; | ||
| ingredients.reduce((acc: any, cur: TIngredient) => { |
There was a problem hiding this comment.
🔴 Нужно исправить:
Следует задать конкретную типизацию для параметра acc - список ингредиентов
| ({ title, ingredients, onClick }, ref) => { | ||
|
|
||
| //не понимаю как исправить | ||
| //@ts-ignore | ||
| const { ref1, ref2 } = ref.current; |
There was a problem hiding this comment.
Попробуйте рассмотреть решения в данном тикете на гитхабе: facebook/react#13029
Возможно, имеет смысл создать дополнительный хук, как предлагается тут: facebook/react#13029 (comment)
| @@ -33,13 +39,14 @@ export const IngredientCard = ({ item, index }) => { | |||
| return; | |||
| } | |||
|
|
|||
| //@ts-ignore | |||
| const dragIndex = item.index; | |||
There was a problem hiding this comment.
🔴 Нужно исправить:
Следует задать типизацию для useDrag как например ниже, чтобы исключить отключение типизации:
| const dragIndex = item.index; | ||
| const hoverIndex = index; | ||
| if (dragIndex === hoverIndex) { | ||
| return; | ||
| } | ||
| const hoverBoundingRect = ref.current.getBoundingClientRect(); | ||
| const clientOffset = monitor.getClientOffset(); | ||
| const clientOffset: any = monitor.getClientOffset(); |
There was a problem hiding this comment.
🔴 Нужно исправить:
В таком случае вместо any можно вручную указать TS какой тип ему использовать:
monitor.getClientOffset() as XYCoord)
|
|
||
| type T1 = MakeOptional<ModalProps, 'title'>; | ||
|
|
||
| export const Modal: React.FC<PropsWithChildren<T1>> = ({ children, onClose, title}) => { |
There was a problem hiding this comment.
🟡 Можно улучшить:
Очень удобно вынести React.FC<PropsWithChildren<..>> в отдельный хук, чтобы переиспользовать в проекте
src/utils/burger-api.ts
Outdated
|
|
||
| type TRefreshResponce = TServerResponce<{ refreshToken: string, accessToken: string}> | ||
|
|
||
| export const checkResponse = (res: TFetchRes) => { |
There was a problem hiding this comment.
🔴 Нужно исправить:
В текущем виде функция checkResponse не конкретизирует тип возвращаемого значения в теле ответа, поэтому все последующие функции связанные с ней работают с any, что не позволяет TypeScript отследить корректность вашего кода.
Т.к. вы уже выше определили универсальный тип ответа, то его стоит применить в соответствующих функциях:
const checkResponse = <T>(res: Response) => {
return res.ok ? res.json().then(data => data as TResponse<T>) : Promise.reject(res.status);
};Аналогично затем следует скорректировать checkSuccess и request, чтобы она тоже возвращала корректный тип
There was a problem hiding this comment.
Здравствуйте!
Вы успешно внесли множество изменений и улучшений в ваш проект, так держать, однако остался момент, который следует также исправить для сдачи проекта в данном спринте:
- Не следует полностью отказываться от начальной типизации запросов, предложенной вами ранее, т.к. в текущем варианте остаются сложности с типизацией возвращаемого результата (он будет
any, что ограничивает проверку тайпскрипта). Пожалуйста, рассмотрите пример отмеченный в комментарии к прошлому ревью
Хорошего вам дня и настроения! 🐈
src/utils/burger-api.ts
Outdated
| type TRefreshResponce = TServerResponce<{ refreshToken: string, accessToken: string}> | ||
|
|
||
| export const checkResponse = (res: TFetchRes) => { | ||
| const checkResponse = (res: TFetchRes): Promise<any> => { |
There was a problem hiding this comment.
🔴 Нужно исправить:
Не следует отказываться от вашей типизации выше, т.к. в текущем варианте остаются сложности с типизацией возвращаемого результата (он будет any, что ограничивает проверку тайпскрипта). Пожалуйста, рассмотрите пример отмеченный в комментарии к прошлому ревью
No description provided.