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

[장바구니 협업 Step 2] 우디(류정우) 미션 제출합니다. #167

Merged
merged 61 commits into from
Jun 11, 2023

Conversation

jw-r
Copy link

@jw-r jw-r commented Jun 5, 2023

배포 링크


안녕하세요 포코~!
우디입니디!


옵티미스틱 업데이트

step1에서는 장바구니 관련 비동기 처리에 모두 옵티미스틱 업데이트를 적용했어요

step2에서 그 형태를 유지하려니 장점보다 단점이 많은 것 같아요

우선 에러가 발생했을 때, 이전의 값으로 되돌려야 한다는 점에서 에러 처리가 복잡해집니다

크리티컬하지 않더라도 굳이 옵티미스틱을 적용할 필요가 있을까 싶습니다

예를 들어, 배민 상회는 장바구니에 담긴 상품의 수량을 변경하는 로직에 옵티미스틱 업데이트를 적용했습니다

때문에 네트워크 문제가 생긴 상황에서도 수량이 변경되는 것을 확인할 수 있지만, 네트워크가 재연결되었을 때 한번 더 요청을 보내지 않는 이상 실제로 변경이 이루어지지 않아요

인터넷이 느릴 때 장점이 극대화 될 것이라고 생각했지만, 에러가 발생한 상황에서도 요청을 보낸 만큼 처리는 이루어지기 때문에 오히려 단점에 가깝다고 느꼈습니다

크리티컬하지 않을지도 모르겠지만, 사용자에게 혼동을 주기에는 충분하다는 생각이 들어요

적절한 Error 컴포넌트를 보여주고 재시도를 하도록 유도하는 것이 더 적절하지 않을까요?

장점으로는 수량 변경과 같이 반복적으로 일어나는 비동기 처리에서, 요청을 기다렸다가 업데이트를 시켜준다면 사용자 입장에서 답답함을 느낄텐대 옵티미스틱 업데이트를 적용하면 그러한 문제가 일어나지 않는다는 것이었어요

하지만 요즘처럼 인터넷이 빠른 시대에 기다리면 얼마나 기다릴까요

수량 변경에도 사실 기다리는 느낌도 들지 않을 것이고,
기다림이 필요한 경우, 적절한 애니메이션과 함께 지루함을 없애주는 UX를 구현해 두는 편이 더 안정성있는 애플리케이션이 아닐까요?

이에 대한 포코의 생각이 궁금합니다!


이번 미션은 개인적으로 어렵고, 시간이 매우 부족했었는데요
그 이유는 제 학습 방법에 문제가 있었던 것이 아닐까 생각합니다

저는 새로운 것을 학습할 때, 익숙해지는 것이 중요하다고 생각했어요

그래서 간단한 사용법을 먼저 숙지한 후, 코드에 적용하고 이것만으로 문제 해결이 되지 않을 때 더 나은 방법이 없는지 찾아봅니다

그러다보니 코드를 전부 갈아 엎어야하는 상황이 빈번히 발생해요

문제를 해결할 때, 제 머릿속에 A와 B만 있으니 이 둘을 활용해서 어떻게든 해결했다가 C라는 보다 나은 해결책이 머리속에 들어오면 모두 바꿔줘야 하니까요

레벨1에서도 느꼈던 것 같은데, 정해진 기한에 맞추기 위해 급하게 구현을 시작하는 것이 오히려 더 시간을 오래 걸리게 하는 것 같아요

앞으로는 �일단 공식 문서를 가볍게라도 모두 읽으며 어떤 문제를 해결할 수 있는지 인지하고, 필요할 때 다시 그 부분에 대해서 학습해보려고 합니다

각각의 장단점이 있는 것 같아요
전자는 시행착오를 몸으로 직접 겪으며 왜 이런 것이 필요하게 되었는지 보다 깊게 이해할 수 있지만 시간이 오래 걸리고,
후자는 빠르게 문제를 해결할 수 있지만, 사용만 할 줄 아는. 뎁스1의 상태에 머무르게 될 수 있을 것 같습니다

이에 대해 포코의 의견이 궁금합니다!


마지막 리뷰 잘 부탁드리겠습니다🤩

jw-r added 30 commits June 5, 2023 16:33
@pocojang pocojang self-assigned this Jun 5, 2023
@pocojang pocojang self-requested a review June 5, 2023 14:47
Copy link

@pocojang pocojang left a comment

Choose a reason for hiding this comment

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

반갑습니다
@evencoding 👋

이번 미션은 다들 코드량이 확~~! 늘어서 리뷰하는데 꽤 오래걸렸네요ㅠㅠ
정말 죄송합니다!

피드백

명령형 갱신 코드

우디의 코드에서 fetchCartItems(), updateQuantity(), refresh()의 공통점이 무엇인지 아실까요?
바로 수동 갱신이라는 것입니다!

코드의 전체적인 설계가 모두 명령형으로 작성되었기 때문에, 갱신이 필요한 시점에 위의 함수를 항상 특정 시점에 개발자가 알아서 호출해야 합니다.
어디서부터 잘못된 것일까요?
이런 코드들은 협업하기에 정말 어려운 코드들입니다.
사람은 언제나 실수를 하고 이런 휴먼 에러가 쌓이면 코드를 갈아엎어야 될 수 있어요..!

이런 코드에 대해 여러 크루들에게 털어놓고 좋은 방법을 찾아보시면 좋겠습니다

비동기 통신

비동기 통신을 위한 전용 클라이언트 함수를 만드셨는데 그런데도 관리가 잘 되고 있지 않네요..
특히 매번 Authorization를 하드 코딩하는 것은 정말 좋지 못합니다.

추가적으로 react-error-boundary를 사용하셨는데.. 이점을 잘 느낄 수 없어 아쉬웠습니다... 애초에 왜 사용하신건지 잘 모르겠어요
🚨 근데 이거 사용 위반 아닌가요?! 🚨

컴포넌트 관리

도메인 가득한 컴포넌트와 무한 div, span이 반복되고 있습니다.
Primitive UI 라이브러리인 Radix UI, Rebass, Chakra UI 등을 참고하셔서 어떤 방법으로 코드를 작성하는지 배워보세요 :)

컴포넌트 내부 값 관리

컴포넌트 내부의 값이.. 거의 매번 JSX에 인라인으로 들어가 있습니다 ㅠㅠ
가격, 날짜, svg 등 컴포넌트 내부에서 반복되는 불필요한 인라인 코드들을 잘 관리해보시는게 어떨까요?

Q&A

Q1. 낙관적 업데이트

꽤나 의미있는 고민을 하신 것 같아요.
서버로부터 응답을 받기 전에 UI를 갱신한다는 것은 정말 어렵고 귀찮은 문제라 대부분 라이브러리들을 사용하기도 하죠.
근데 이것이 애니메이션과의 비교 대상은 아닌 것 같아요.
낙관적 업데이트는 당연한거고 거기에 애니메이션까지 한 스푼 더 한다라고 생각하는게 더 완벽해지는 프론트엔드 앱의 관점인 것 같아요

Q2. 학습 방법 문제

A, B, C + Depth1 이라는 예시를 들어 학습 방법에 대해 설명해주셨는데 좋네요.
이정도시면 이미 객관화가 잘 되어 있는 것 같습니다.

학습에 이렇게 많은 고민이 든다는 것은 결국 잘하고 싶어서겠죠?
그럼 어떻게 잘하면 될까요?
사실.. 잘 하고 싶은 그것을 계속 반복하기만해도 됩니다.

음.. 다시 본론으로 돌아와서 예시로 들어주신 A, B, C는 Generalist로 생각해볼 수 있고 Depth는 Specialist로 생각해볼 수 있습니다.
Generalist & Specialist가 적절하게 혼합된 T자형 인재를 찾아보시면 앞으로 학습 방향에 큰 도움이 될 겁니다.

또한 기한에 맞춰서 급하게 무언가 구현한다는 것에 죄책감을 가지지 않으셨으면 좋겠습니다.
기한에 맞춰서 구현하는 것은 정말 중요한 일입니다. 대신 그 과정에서 학습이 생략되고 있다면 그것을 저는 기술 부채라고 정의합니다.
그리고 그 기술 부채를 갚는 행위를 학습이라고 정의합니다 ㅎㅎ

어차피 개발자들은 추상화된 언어와 라이브러리, 프레임워크, 구글링, Stack Overflow, AI 등과 함께 이미 개발 지식이 없어도 많은 것을 할 수 있습니다.
결국 네트워크 기본 지식이 1도 없는 사람도 클론코딩만 보고 따라하면 Fetch API, Axios와 함께 누구나 비동기 통신을 할 수 있지 않나요?

그러니 너무 학습에 스트레스 받지 마세요..!
고민보다는 실행이 중요합니다.
때로는 기술에 대한 빚을 지고 때로는 그 빚을 갚으세요!

제가 너무 주절 주절했는데 요약해보자면..

  • Generalist & Specialist => T자형 학습
  • Input & Output => 때로는 Output, 때로는 Input 의식적인 왕복 학습
  • 기술 부채를 빌리고 갚는 행위에 대한 반복 => 죄책감을 가지지 않으면서 또 때로는 죄책감을 가지며 정신 수련하기
  • 다 필요없고 잘하고 싶은 그것을 계속 반복하기

아래는 학습에 굉장히 도움되는 글과 책입니다.

https://ebook.insightbook.co.kr/book/65
https://ahnheejong.name/articles/programmers-learning


이만 리뷰 마치도록 할게요!
많은 코드 작성하시느라 고생하셨습니다~

"react": "^18.2.0",
"react-dom": "^18.2.0",
"react-error-boundary": "^4.0.9",
Copy link

Choose a reason for hiding this comment

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

후덜덜.. 용감하시군요

Copy link
Author

Choose a reason for hiding this comment

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

에러 핸들링이 생소하기도 하고, 다른 부분에 시간을 많이 쏟아서.. 급하게 도입해버렸습니다😭
'안하는 것 보단 낫다!' 라고 생각해서 그만🫠

방학기간에 레벨2에서 학습한 내용을 심화적으로 학습할 수 있도록 앱을 하나 만들 계획인데, 그 때 꼭 직접 Class로 작성해서 천천히 맛보겠습니다🥺

src/components/FallbackRender/FallbackRender.tsx Outdated Show resolved Hide resolved
Comment on lines 15 to 17
{/* <option value="MSW">MSW</option> */}
<option value="이리내">이리내</option>
<option value="채채">채채</option>
Copy link

Choose a reason for hiding this comment

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

대놓고.. 이런 하드코딩을..ㅠㅠ

Copy link
Author

Choose a reason for hiding this comment

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

이렇게 치명적일수가..🫠
수정했습니다!!

Comment on lines 20 to 31
<img
src="data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iMjQiIGhlaWdodD0iMjQiIHZpZXdCb3g9IjAgMCAyNCAyNCIgZmlsbD0ibm9uZSIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIj4gPHBhdGggZmlsbC1ydWxlPSJldmVub2RkIiBjbGlwLXJ1bGU9ImV2ZW5vZGQiIGQ9Ik03Ljg3NSAyMkM4LjkxMDUzIDIyIDkuNzUgMjEuMTYwNSA5Ljc1IDIwLjEyNUM5Ljc1IDE5LjA4OTUgOC45MTA1MyAxOC4yNSA3Ljg3NSAxOC4yNUM2LjgzOTQ3IDE4LjI1IDYgMTkuMDg5NSA2IDIwLjEyNUM2IDIxLjE2MDUgNi44Mzk0NyAyMiA3Ljg3NSAyMloiIHN0cm9rZT0iIzMzMzMzMyIgc3Ryb2tlLXdpZHRoPSIyIiBzdHJva2UtbGluZWNhcD0icm91bmQiIHN0cm9rZS1saW5lam9pbj0icm91bmQiLz4gPHBhdGggZmlsbC1ydWxlPSJldmVub2RkIiBjbGlwLXJ1bGU9ImV2ZW5vZGQiIGQ9Ik0xNy44NzUgMjJDMTguOTEwNSAyMiAxOS43NSAyMS4xNjA1IDE5Ljc1IDIwLjEyNUMxOS43NSAxOS4wODk1IDE4LjkxMDUgMTguMjUgMTcuODc1IDE4LjI1QzE2LjgzOTUgMTguMjUgMTYgMTkuMDg5NSAxNiAyMC4xMjVDMTYgMjEuMTYwNSAxNi44Mzk1IDIyIDE3Ljg3NSAyMloiIHN0cm9rZT0iIzMzMzMzMyIgc3Ryb2tlLXdpZHRoPSIyIiBzdHJva2UtbGluZWNhcD0icm91bmQiIHN0cm9rZS1saW5lam9pbj0icm91bmQiLz4gPHBhdGggZD0iTTUuMjQ1NDUgNi4xNjY1NkgyMUwxOS40NzI3IDEzLjE1ODFDMTkuMzAxMSAxMy45NDk5IDE4LjUzNTEgMTQuNTE1MiAxNy42NTQ1IDE0LjQ5OTdIOC4wNDU0NUM3LjEyNjggMTQuNTA2OCA2LjM0NjY3IDEzLjg4NDcgNi4yMjcyNyAxMy4wNDk3TDQuODQ1NDUgMy40NDk5NkM0LjcyNjkzIDIuNjIxMzEgMy45NTcxOSAyLjAwMTI3IDMuMDQ1NDUgMkgxIiBzdHJva2U9IiMzMzMzMzMiIHN0cm9rZS13aWR0aD0iMiIvPiA8L3N2Zz4g"
alt="Cart Size Icon"
/>
<span>장바구니</span>
<CartSize />
</styled.CartPageLink>
<styled.OrdersPageLink to="/orders">
<img
src="data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iMjQiIGhlaWdodD0iMjQiIHZpZXdCb3g9IjAgMCAyNCAyNCIgZmlsbD0ibm9uZSIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIj4gPHBhdGggZD0iTTE2IDdDMTYgOS4yMDkxNCAxNC4yMDkxIDExIDEyIDExQzkuNzkwODYgMTEgOCA5LjIwOTE0IDggN0M4IDQuNzkwODYgOS43OTA4NiAzIDEyIDNDMTQuMjA5MSAzIDE2IDQuNzkwODYgMTYgN1oiIHN0cm9rZT0iIzMzMzMzMyIgc3Ryb2tlLXdpZHRoPSIyIi8+IDxwYXRoIGQ9Ik0yMSAyMUMyMSAxNS40NzcyIDE2Ljk3MDYgMTEgMTIgMTFDNy4wMjk0NCAxMSAzIDE1LjQ3NzIgMyAyMSIgc3Ryb2tlPSIjMzMzMzMzIiBzdHJva2Utd2lkdGg9IjIiLz4gPC9zdmc+IA=="
alt="User Icon"
/>
Copy link

Choose a reason for hiding this comment

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

이미지를.. 이렇게 방치해놓다니.. 씁쓸하네요 😭

Copy link
Author

Choose a reason for hiding this comment

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

이 부분도 미처 챙기지 못했었네요🥲
재사용성 등을 위해, svg 태그 형식으로 변환하고 리액트 컴포넌트 형식으로 추상화 해줬습니다!


import { useBaseApiUrlState } from '@recoils/baseApiUrlAtoms';

export const ApiSelector = () => {
Copy link

Choose a reason for hiding this comment

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

컴포넌트 네이밍.. 논란이 많아보이네요..

Copy link
Author

Choose a reason for hiding this comment

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

atom의 selector와 매우 혼동되는군요..
수정했습니다!

Choose a reason for hiding this comment

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

잘못된건 아닌 것 같아요 ㅎㅎ
Selector라는 의미가 Recoil, Query Selector 등 다양하게 쓰이다보니 그런 맥락도 고려해보라는 이야기정도였습니다요~

<styled.OrderBoxHeader>
<div>
<span>주문번호 : {orderId}</span>
<styled.OrderDate>주문날짜 : {orderDate.split('.')[0]}</styled.OrderDate>
Copy link

Choose a reason for hiding this comment

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

음.. 어딘가에 변수로 만들어놓고 사용할 수 있지 않을까요?

const [orderDateValue] = orderDate.split('.')

Copy link
Author

Choose a reason for hiding this comment

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

오홍 확실히 현재 지나치게 명령형인 느낌이 드네요

부모 컴포넌트에서 변수로 만들고 props로 내려주도록 변경했습니다!

body?: object;
}

export const fetchAPI = async (url: string, optionalProps?: OptionalProps) => {
Copy link

Choose a reason for hiding this comment

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

조금만 더 발전시키면 좋을 것 같은데 아쉽군요!!

Copy link
Author

Choose a reason for hiding this comment

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

저도 fetchAPI가 많이많이 �아쉬웠어요😭
Client에서 비동기 로직에 대한 에러 핸들링을 다루는 것이 거의 처음이라 특히 아쉬웠던 것 같아요

에러 핸들링에 대해 조금 더 공부하고, 다른 코드들을 많이 참고하며 보다 나은 함수로 리펙토링 해보겠습니다!!

Choose a reason for hiding this comment

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

좋아요~~ 앞으로 잘하면 되죠!!

throw new Error(unKnownErrorMessage);
}

if (contentType === 'application/json') {
Copy link

Choose a reason for hiding this comment

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

application/json 분기를 만들게 된 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

요청의 Response에 혹여나 json 데이터를 넘겨받지 않은 경우에는 SyntaxError가 발생하기 때문에 감싸줬습니다

try, catch 문을 사용하는 편이 더 적절할 수 있다고 생각해요

하지만 현재 fetchAPI 가 다소 아쉬운 모습을 하고 있어서인지..

비동기 로직 전체를 try, catch문으로 감싸기에는 서버에서 전달받은 에러와 구분하기가 어렵다고 생각했습니다

그렇다고 json 변환을 하는 부분만 감싸주기 보다는 contentType을 검사하는 편이 가독성이 좋아질 것이라고 판단했어요!

Choose a reason for hiding this comment

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

아하.. 신기하네요
이렇게까지 고려하신 분은 처음 보지만 일단 좋습니다 👍

src/api/fetchAPI.ts Outdated Show resolved Hide resolved
interface OptionalProps {
method?: string;
headers?: HeadersInit;
body?: object;
Copy link

Choose a reason for hiding this comment

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

object이라는게 있었군요..?

Copy link
Author

@jw-r jw-r Jun 8, 2023

Choose a reason for hiding this comment

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

네 객체를 대상으로 하는 any와 타입처럼 동작하는 걸로 알고 있습니다😭

interface OptionalProps {
  method: string;
  headers: HeadersInit;
  body: OrdersRequestBody | AddCartRequestBody | UpdateQuantityRequestBody;
}

위와 같이 body로 올 수 있는 타입을 모두 지정해줬는데, 이것이 좋은지는 잘 모르겠습니다
body를 보내야 하는 경우가 늘어나면 계속 타입을 추가해줘야 하는 걸까요?

Choose a reason for hiding this comment

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

나중에는 이런타입 만들지마시고 내장 타입을 잘 활용해보세요!!

jw-r and others added 16 commits June 8, 2023 21:02
Co-authored-by: Poco <devjang2016@gmail.com>
@jw-r
Copy link
Author

jw-r commented Jun 8, 2023

포코 안녕하세요!
너무너무 꼼꼼하고 소중한 리뷰 감사합니다🤩

하필 방학이 다가와서..시간이 부족해서 기껏해야 포코가 달아준 코맨트를 우선적으로 해결했습니다!

명령형 갱신 코드, 비동기 통신, 컴포넌트 관리, 컴포넌트 내부 값 관리 등 저에게 너무 �필요했던 내용들인데..
이 부분도 빠르게 학습해서 추후 코드에 반영해보겠습니다😆

Copy link

@pocojang pocojang left a comment

Choose a reason for hiding this comment

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

@evencoding 👋
우테코 정규과정 대장정 드디어 끝났군요
그동안 고생 많으셨습니다.


이미 대부분의 피드백은 앞의 핑퐁에서 많이 드렸기때문에 이만 머지하도록 합니다!
한 가지 말씀드리고 싶은건 벌써부터 Recoil에 대해 불편함을 느끼신다는 것이 성장에 긍정적인 것 같네요.

이제 그 다음 스텝은 불편함을 느끼는 라이브러리가 왜 우리를 불편하게 찾아보시면 좋아요.
생각보다 저장소의 Issue, Discussion에 나랑 비슷한 생각을 하는 사람들이 많더라고요.
그리고 그런 것들을 하나 하나 찾아서 읽다보면 생각도 넓어지고 성장에 도움이 되실거에요.

만약 불편함을 느꼈다면 애초에 그 라이브러리랑 나랑 맞거나 공식문서를 제대로 확인안했거나더라고요ㅎㅎ
라이브러리는 결국 어떠한 기술적인 철학을 강조하기때문이죠!


모든 피드백 하나하나 소중하게 확인하는 모습 정말 멋졌습니다.
그럼 방학에 충전 잘하세요
화이팅!!

@pocojang pocojang merged commit 01ff13c into woowacourse:evencoding Jun 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants