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

[1단계 - 장바구니] 호프(김문희) 미션 제출합니다. #71

Merged
merged 56 commits into from
May 15, 2022

Conversation

moonheekim0118
Copy link

@moonheekim0118 moonheekim0118 commented May 12, 2022

안녕하세요 발리스타님! 처음뵙겠습니다 호프라고합니다 ✨ ✨ 이번 미션에서 만나뵙게 되어 영광이에요 😃
부족한 코드이지만 잘부탁 드립니다 👍 👍 👍
이번에 리덕스를 적용해서 장바구니 페이지를 구현해보았습니다 :)
리덕스를 이렇게 사용하는게 올바른지 아직 의문이 들지만, 가감없는 피드백 부탁드립니다. 🙇 🙇

데모 페이지

구현 목록

필수 요구사항

  • Redux를 활용한 상태 관리
  • REQUIREMENTS.md에 요구 사항 도출
  • Mock Data 활용 (Schema 설계까지)

필수 구현 페이지

  • 상품 목록

추가 구현 사항

  • 상품 상세 페이지
  • 장바구니 담기 기능 (새로고침 시 초기화)


사용한 기술 목록

  • React
  • Redux
  • Redux-thunk
  • Styled-Component
  • Storybook


src 폴더 구조

📦src
 ┣ 📂assets
 ┃ ┣ 📂mock
 ┃ ┃ ┗ 📜index.js
 ┃ ┣ 📂png
 ┃ ┃ ┣ 📜emptyImg.png
 ┃ ┃ ┣ 📜errorApiImg.png
 ┃ ┃ ┗ 📜notFoundImg.png
 ┃ ┗ 📂svg
 ┃ ┃ ┣ 📜bigCart.svg
 ┃ ┃ ┣ 📜smallCart.svg
 ┃ ┃ ┗ 📜spinner.svg
 ┣ 📂components
 ┃ ┣ 📂Button
 ┃ ┃ ┣ 📜Button.jsx
 ┃ ┃ ┗ 📜Button.stories.js
 ┃ ┣ 📂Header
 ┃ ┃ ┣ 📜Header.jsx
 ┃ ┃ ┗ 📜Header.stories.js
 ┃ ┣ 📂ImgWrapper
 ┃ ┃ ┣ 📜ImgWrapper.jsx
 ┃ ┃ ┗ 📜ImgWrapper.stories.js
 ┃ ┣ 📂Layout
 ┃ ┃ ┗ 📜Layout.jsx
 ┃ ┣ 📂MenuItem
 ┃ ┃ ┣ 📜MenuItem.jsx
 ┃ ┃ ┗ 📜MenuItem.stories.js
 ┃ ┣ 📂ProductContainer
 ┃ ┃ ┣ 📜ProductContainer.jsx
 ┃ ┃ ┗ 📜ProductContainer.stories.js
 ┃ ┣ 📂ProductDetail
 ┃ ┃ ┣ 📜ProductDetail.jsx
 ┃ ┃ ┗ 📜ProductDetail.stories.js
 ┃ ┣ 📂ProductItem
 ┃ ┃ ┣ 📜ProductItem.jsx
 ┃ ┃ ┗ 📜ProductItem.stories.js
 ┃ ┗ 📂Skeleton
 ┃ ┃ ┣ 📜Skeleton.jsx
 ┃ ┃ ┗ 📜Skeleton.stories.js
 ┣ 📂hooks
 ┃ ┗ 📜useReduxState.js
 ┣ 📂pages
 ┃ ┣ 📂Cart
 ┃ ┃ ┗ 📜Cart.jsx
 ┃ ┣ 📂NotFound
 ┃ ┃ ┗ 📜NotFound.jsx
 ┃ ┣ 📂Product
 ┃ ┃ ┗ 📜Product.jsx
 ┃ ┣ 📂ProductList
 ┃ ┃ ┗ 📜ProductList.jsx
 ┃ ┗ 📜index.js
 ┣ 📂reducers
 ┃ ┣ 📂cart
 ┃ ┃ ┣ 📜cart.actionTypes.js
 ┃ ┃ ┣ 📜cart.actions.js
 ┃ ┃ ┗ 📜cart.reducer.js
 ┃ ┣ 📂product
 ┃ ┃ ┣ 📜product.actionTypes.js
 ┃ ┃ ┣ 📜product.actions.js
 ┃ ┃ ┣ 📜product.reducer.js
 ┃ ┃ ┗ 📜product.thunks.js
 ┃ ┣ 📂products
 ┃ ┃ ┣ 📜products.actionTypes.js
 ┃ ┃ ┣ 📜products.actions.js
 ┃ ┃ ┣ 📜products.reducer.js
 ┃ ┃ ┗ 📜products.thunks.js
 ┃ ┗ 📜rootReducer.js
 ┣ 📂store
 ┃ ┗ 📜configureStore.js
 ┣ 📂utils
 ┃ ┗ 📜apiClient.js
 ┣ 📜App.js
 ┗ 📜index.js

Mock Data 구조

{
  "products": [
    {
      "id": number,
      "name": string,
      "imgUrl": string,
      "price": string,
      "quantity": number
    },
  ],
  "cart-products": [],
}

질문

  • 현재 구현된 사항은 아니지만, 궁금한 점이 있습니다. 리덕스 스토어에 장바구니 아이템 리스트를 가져왔고, 만약에 사용자가 장바구니 수량을 변경한다고 했을 때, 스토어 상태 변경(수량변경) / API 수량 변경 요청 을 따로 따로 해주는게 맞을까요 ? 리덕스 스토어를 사용해서 상태관리를 한다면 이 스토어에 저장된 상태를 직접 변경해주고, API 요청을 해주는게 맞을까요? 아니면 API 요청을 해주고, 요청 후 받은 응답값으로 스토어 상태를 업데이트 해주는게 맞을까요 ?

    • 스토어에 저장된 상태를 직접 변경해 준 후, 따로 API 요청(PUT)을 해줄 경우
      • 만약 서버에서 해당 요청이 제대로 처리되지 않으면.. 사용자는 API 요청이 처리되지 않았다는 것을 모를 것 같아서 좋지 않은 방법 같아요 ㅠㅠ 다만, api 요청을 최소화 할 수 있기 때문에 성능상으로는 좋은 방법 같아요.
    • API 요청을 해주고, 요청 후 받은 응답값으로 스토어 상태를 업데이트 해줄 경우
      • 정확한 서버 응답값을 스토어에 반영할 수 있겠지만, 그 만큼 사용자 반응을 UI 가 느리게 반영 할 수 있을 것 같아서 좋지 않을 것 같아요 ㅠㅠ
  • 현재 메인 페이지에서 useEffect 로 첫 렌더링 시 상품 리스트를 서버로부터 가져오도록 구현해놓았는데요, 만약에 store 에 상태가 저장되어있다면 서버 요청을 보내지 않도록 임시적으로 구현하여 불필요한 api 요청을 제거했습니다. 그런데 만약 페이지네이션이 들어가면 더 복잡해질 문제라고 생각하는데요, 해당 페이지 첫 렌더링 시 데이터를 요청해주는게 맞을까요?

moonheekim0118 and others added 25 commits May 10, 2022 15:46
Co-authored-by: wonsss <bigdatajang@gmail.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
- thunk, axios, react-redux 설치

Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Co-authored-by: Marco <wonsss@users.noreply.github.com>
Copy link

@Vallista Vallista left a comment

Choose a reason for hiding this comment

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

안녕하세요 호프님 :)
1단계 코드 잘 보았습니다 :)
앞으로 미션동안 잘 부탁드려요!

피드백

  • 컴포넌트를 딱 적당한 사이즈로 나눠주신 것 같습니다 :) 보면서 잘 만드셨다는걸 느낄 수 있었습니다 완전 칭찬해 짝짝짝!!
  • 커스텀 훅으로 useReduxState.js를 해준것도 좋았습니다 :)
  • 여기서 조금 더 제안드리는 것은, dispatch, state, 등을 한번에 가져올 수 있도록 비즈니스 훅을 만드는게 어떨까 제안드립니다 :) 지금은 페이지에 바로 묶여있는데, 그 상위에서 요소들을 제어하는게 어떨까요? 상위에서 제어하게 되면 많은 장점이 존재하는데, 가장 큰 것은 "데이터가 상위로 감으로써, 페이지는 레이아웃을 상태에 따라 조합하도록 집중" 할 수 있습니다. 일종의 페이지도 컴포넌트 개념으로 확장할 수 있는 것이죠.
  • 그 외에는 따로 피드백은 없구, 코드 라인에 상세하게 피드백 남겨두었습니다 :) 1단계에선 전반적인 아키텍처링을 보았는데요~ 지금 당장은 적당한 사이즈로 보입니다. 2단계에선 현재 덕스 패턴을 기조로 한 redux 와 이들이 더 추가된 요구사항으로 하여금 어떻게 확장되고 문제가 생기는지 확인후, 피드백을 드리도록 하겠습니다.

질문 답변

Q1

현재 구현된 사항은 아니지만, 궁금한 점이 있습니다. 리덕스 스토어에 장바구니 아이템 리스트를 가져왔고, 만약에 사용자가 장바구니 수량을 변경한다고 했을 때, 스토어 상태 변경(수량변경) / API 수량 변경 요청 을 따로 따로 해주는게 맞을까요 ? 리덕스 스토어를 사용해서 상태관리를 한다면 이 스토어에 저장된 상태를 직접 변경해주고, API 요청을 해주는게 맞을까요? 아니면 API 요청을 해주고, 요청 후 받은 응답값으로 스토어 상태를 업데이트 해주는게 맞을까요 ?

- 스토어에 저장된 상태를 직접 변경해 준 후, 따로 API 요청(PUT)을 해줄 경우
만약 서버에서 해당 요청이 제대로 처리되지 않으면.. 사용자는 API 요청이 처리되지 않았다는 것을 모를 것 같아서 좋지 않은 방법 같아요 ㅠㅠ 다만, api 요청을 최소화 할 수 있기 때문에 성능상으로는 좋은 방법 같아요.

- API 요청을 해주고, 요청 후 받은 응답값으로 스토어 상태를 업데이트 해줄 경우
정확한 서버 응답값을 스토어에 반영할 수 있겠지만, 그 만큼 사용자 반응을 UI 가 느리게 반영 할 수 있을 것 같아서 좋지 않을 것 같아요 ㅠㅠ

저희 프로젝트인, B마트 장바구니에서 수량을 증가시킬때 동일한 동작을 진행하고 있습니다. 저도 그 부분에 대해서 고민이 많았는데요! 지금 저희 팀의 프로덕트는 후자로 작동하고 있습니다.

하지만, 최근 비마트에서 포캣몬빵을 팔기 시작하면서 장바구니에서 유저분들이 9시 오픈인데 8시부터 수량을 엄청 올리고 내리고를 반복하고 있습니다. 그럼에따라 8~11시까지 유저들이 미친듯이 오픈런을 시행해 트래픽이 몰리고 있는 상황이에요.

그래서 전자로 변경할까 고민하고 있습니다. 전자로 변경할 때는 다음과 같이 개발할 예정이에요.

  1. 유저가 수량을 클릭할 때 API를 보내지 않고 기다린다.
  2. 유저가 아이템별 수량에 대한 버튼을 0.2초 이상(이는 유저가 계속 request를 얼만큼의 빈도로 보내는지 측정이 필요합니다.) 누르지 않았을 경우 API 요청. (디바운스로 마지막 이벤트만 보내도록.)

요것은 프로젝트의 성격과 니즈에따라 다르므로, 호프님이 진행하고 계시는 프로젝트는 서버 스펙이 어느정도고, 어떤 성향의 프로젝트인지 판단하시고 적용하시면 되어보입니다 :) 저는 단순하게 후자로 해도 괜찮을 것 같아요. 쓰로틀로 0.1초 줘도 큰 문제 없을듯!

Q2

현재 메인 페이지에서 useEffect 로 첫 렌더링 시 상품 리스트를 서버로부터 가져오도록 구현해놓았는데요, 만약에 store 에 상태가 저장되어있다면 서버 요청을 보내지 않도록 임시적으로 구현하여 불필요한 api 요청을 제거했습니다. 그런데 만약 페이지네이션이 들어가면 더 복잡해질 문제라고 생각하는데요, 해당 페이지 첫 렌더링 시 데이터를 요청해주는게 맞을까요?

store에 key - data 형태로 들고 있다가 주면 페이지네이션도 괜찮지 않을까요? key-{page} 형태로..?

@@ -0,0 +1,19 @@
module.exports = {

Choose a reason for hiding this comment

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

eslintrc.json과 .js. 어떤 차이가 있고 왜 두 가지를 제공하는걸까요?

Copy link
Author

Choose a reason for hiding this comment

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

찾아보니, eslint 파일 설정 포맷은 js , json을 제외하고도 JavaScript (ESM), yaml 도 제공해준다고 합니다. 각각은 아래와 같이 사용됩니다.

  • js : eslintrc.js 로 정의 할 수 있고, eslint 설정이 담긴 객체를 export 합니다.
  • JavaScript (ESM) : eslintrc.cjs 로 정의 할 수 있고, package.json 에서 “type”:”module” 을 지정 할 경우 사용합니다.
    • ESM 은 ECMAScript module 의 약자입니다.
    • ESM 은 ECMAScript에서 지원하는 자바스크립트 공식 모듈 시스템입니다.
    • (추측) 노드에서는 아직 common js 를 지원하기 때문에, import ~export문 을 쓰기 위해서는 .cjs 로 정의해줘야하는 것 같아요!
  • YAML : eslint.yaml or eslint.yml 로 정의 할 수 있습니다.
    • YAML 들어만 봤지 무엇인지 처음찾아봤는데 JSON 처럼 데이터를 쉽게 표현하기 위한 언어라고합니다.
  • JSON : eslintrc.json 으로 정의 할 수 있습니다.

그리고 만약 여러개의 설정 파일이 같은 디렉토리에 있다면 우선순위는,

  1. .eslintrc.js
  2. .eslintrc.cjs
  3. .eslintrc.yaml
  4. .eslintrc.yml
  5. .eslintrc.json
  6. package.json

이라구 합니다.

즉 말씀주신 eslintrc.js 와 eslintrc.json의 차이점은 lint 설정 파일 내에서 사용되는 언어 (javascript / json) 과 우선순위가 있겠네요!

그렇다면 왜 이렇게 eslint 가 여러개의 파일 설정 포맷을 제공하는지 추측해보았는데요..! 다양한 환경을 지원해주기 위해서! 라고 생각합니다.
누군가는 백엔드 서버를 구현하는데 eslint 를 사용하기도 하고, 누군가는 클라이언트를 구현하기 위해서 사용하기도 하니까요? 따라서 js, esm 를 둘다 지원해주는 것 같고, json, yaml 을 지원해주는 것은 자바스크립트 모듈 시스템에 구애받지 않는 포맷을 제공해주기 위함인 것 같다고 추측합니다..!!!

참고자료

Configuration Files - ESLint - Pluggable JavaScript linter

Choose a reason for hiding this comment

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

아주 훌륭합니다... 박사가 되셨군요..

@@ -0,0 +1,19 @@
module.exports = {
env: {
browser: true,

Choose a reason for hiding this comment

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

browser와 node 옵션 둘 다 주는 이유가 있을까요?

Copy link
Author

@moonheekim0118 moonheekim0118 May 14, 2022

Choose a reason for hiding this comment

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

일단..! 별 생각 없이 린트를 설정한 것 같아서 해당 옵션이 뭘 해주는지 부터 찾아봤습니다.

Env 옵션은,

  • 스크립트 실행 환경을 설정해줍니다.
  • 전역 변수를 미리 정의해줍니다.

browser : true 로 해준 이유

  • browser 를 환경으로 설정해주지 않을 경우, browser 환경에서 제공하는 메서드들을 (window.*) 인식 할 수가 없기에 에러가 납니다.

node : true 로 해준 이유

  • 이 옵션은 원래 주지 않았다가, eslint 파일을 설정 한 후 eslint 설정 파일 내에서 module is not defined 라는 에러가 나서 준 옵션입니다.
  • 즉, node 환경에서 제공하는 메서드 (module) 을 사용하기 위해서 설정한 옵션입니다.

이를 공부하다가 알게 된 사실인데, Env 옵션에 사용자가 직접 전역 변수를 설정할 수 도 있네요!

좋은 질문 감사드립니다 ㅎㅎ

참고자료

Language Options - ESLint - Pluggable JavaScript linter

Copy link

@Vallista Vallista May 15, 2022

Choose a reason for hiding this comment

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

아휴~~ 좋은 답변 감사합니다!

완전 박사가 되신듯..

module.exports = {
env: {
browser: true,
es2021: true,

Choose a reason for hiding this comment

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

es2021를 true 로 했을때 장점이 무엇일까요?

Copy link
Author

Choose a reason for hiding this comment

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

위의 답변 에 대해 답하고 나니 이는 쉽게 파악이 되네요! 최신 문법(es2021)의 전역 변수들을 eslint가 자동으로 사전에 설정해놓음으로써 최신문법을 사용 할 수 있겠습니다.

# dependencies
/node_modules
/.pnp
.pnp.js

Choose a reason for hiding this comment

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

pnp.js 는 무엇일까요?

Copy link
Author

@moonheekim0118 moonheekim0118 May 14, 2022

Choose a reason for hiding this comment

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

먼저 결론부터 말하면 pnp 는 yarn 환경에서 쓰이는 것이고, 일종의 패키지들의 룩업테이블 역할을 하는 것 같아요! pnp는 plugNplay 의 약자입니다. Cra 에서 create-react-app —use-pnp Yarn 과 함께 실행하면 pnp 를 사용하는 프로젝트를 생성할 수 있다구 하네요!

그러면 왜 pnp 가 필요할까요 ?! 먼저 yarn 이 해결하려는 문제와 함께 살펴볼게요.

node_modules 문제점

이전에 yarn install 을 실행하면, yarn 은 node_modules 폴더를 생성했습니다. 이 때 Node 는 특정 패키지에 대해서 바로 알지는 못하고, 계속해서 상위 node_modules 파일을 찾아보는 식으로 패키지를 찾았습니다. 그런데 이는 아래와 같은 이유로 매우 비효율적이었어요

  • node_modules에는..정말 정말 엄청난 양의 파일이 들어있습니다. 그래서 애초에 yarn install 후 node_modules를 생성하는 것에 시간 소모가 큽니다.
  • 그 외에도 node_modules 생성은 I/O 부하가 높은 작업이기도 하고, package.json 에 디펜던시를 까먹고 넣지 않으면..! 코드에서 런타임에서 에러가 날 수도 있습니다.
  • 그리고 무엇보다, node_modules 폴더 설계는 패키지 매니저가 적절하게 패키지 중복 제거를 허용하지 않기에 비효율적입니다! 따라서 중복된 패키지가 여러번 존재하게 되는 문제도 있어요.

그런데! Yarn 은 이미 디펜던시 트리에 대해 다~~~ 알고 있습니다. 따라서 패키지가 어디있는지 찾는 역할을 Node 에게 맡기지 않아도 되는거죠! 대신 패키지 매니저로서 패키지의 위치를 인터프리터에 알리고, 패키지 매니저 yarn 은 패키지 및 패키지 버전간의 디펜던시를 관리를 합니다. 이를 위해서 ‘plug(연결)Nplay’ 가 만들어졌어요 (인터프리터와 패키지를 연결한다는 것..이겠죠?)

따라서 yarn 은 2.0 버전부터 node_modules 대신 .pnp.js 파일을 생성합니다.
pnp.cjs 파일은 여러개의 맵들이 포함되어있는데요,

  1. 패키지 이름과 버전을 디스크에서의 위치와 연결해줍니다.
  2. 패키지 이름과 버전을 디펜던시 리스트와 연결해줍니다.

이러한 룩업테이블을 통해서 yarn 은 Node 에게 쉽게 ‘해당 패키지를 찾으려면 어디로가!’ 를 말해줄 수 있어요. 이전에 Node 가 일일이 node_modules 파일을 탐색해야했던 것에 비하면..엄청난 발전입니다.

참고자료

Plug’n’Play | Yarn - Package Manager

Choose a reason for hiding this comment

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

좋아용! 그런데, 지금 프로젝트에 pnp를 쓰고 있고, 사용하는 것일까요!?

Copy link
Author

Choose a reason for hiding this comment

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

아앗 답변만하고 ㅋㅋㅋ적용은 안했네요!! 😆 .. 현재 프로젝트에서 사용되지 않는 것들 다 gitignore에서 걷어내겠습니다..!!

@@ -0,0 +1,6 @@
module.exports = {
singleQuote: true, // 작은 따옴표 사용
trailingComma: 'all', // 꼬리 콤마 사용

Choose a reason for hiding this comment

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

trailingComma: 'all' 는 꼬리 콤마를 어떻게 사용한다는 걸까요? 어떤 옵션이 있고, 왜 all 을 사용하셨을까요?

Copy link
Author

@moonheekim0118 moonheekim0118 May 14, 2022

Choose a reason for hiding this comment

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

꼬리콤마란 무엇이고, 어떤 옵션이 있고, 왜 사용했는지 순으로 답변해보겠습니다!!!

꼬리콤마란

Trailing commas 옵션은 여러 줄의 콤마로 구분되었을 경우 아래와 같이 꼬리 콤마를 붙여줍니다.

// 포맷 전 
const obj = {
  key1: 1,
  key2: 2
};

// 포맷 후 
const obj = {
  key1: 1,
  key2: 2,
};

꼬리콤마 옵션 종류

옵션 종류는 아래와 같아요.

  1. es5 (default) : ES5에서 꼬리콤마가 유효한 경우 콤마를 붙여줍니다. (객체,배열 등등) 하지만 타입스크립트의 타입 파라미터에서는 꼬리콤마를 붙여주지 않습니다.
  2. none : 그 어떤 경우에도 꼬리콤마를 붙여주지 않습니다.
  3. all : 꼬리콤마가 가능한 경우(함수 파라미터 포함) 모두 꼬리콤마를 붙여줍니다. 타입스크립트의 타입 파라미터에서도 꼬리콤마를 붙여줍니다.

왜 all 로 설정했는지?

  • 사실 프리티어 설정할 때 페어와 각자의 이전 프리티어 설정 옵션을 보고 한거라..솔직히 말하면 딱히 큰 이유는 없습니다…ㅎㅎ! 그런데, 저는 None 옵션은 거의 사용하지 않는 이유가 객체나 배열이 길어지면 추후에 요소가 추가될 가능성이 높기 때문에 꼬리콤마를 프리티어에서 알아서 붙여주는 것을 선호해요! 함수 파라미터도 마찬가지이구요..~! 그래서 함수 파라미터 꼬리콤마를 지원하지 않는 es5보다는 all 을 사용하는 게 더 편할 것 같아요 :)

참고자료

Options · Prettier

Choose a reason for hiding this comment

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

아주 훌륭합니다!

src/index.js Outdated

const store = configureStore();

export const GlobalStyle = createGlobalStyle`

Choose a reason for hiding this comment

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

globalstyle 을 별도로 때는건 어떨까요?

Copy link
Author

@moonheekim0118 moonheekim0118 May 14, 2022

Choose a reason for hiding this comment

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

따로 분리하는 편이 훨씬 관리에도 좋겠구, 나중에 스토리북에서도 사용될 때도 유용하게 쓸 수 있을 것 같아요!
그래서 styles/globalStyle.js 로 분리해주었습니다.

Copy link
Author

@moonheekim0118 moonheekim0118 May 14, 2022

Choose a reason for hiding this comment

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

그리고 원래 globalStyle 이 index.js 에서 사용되었는데요! 생각해보니 App.js 에서 사용되는게 더 맞다고 판단했습니다.
index.js 는 단순히 App.js 를 리액트돔에 붙여주는 역할을 한다고 생각하기 때문에 컴포넌트 관련 스타일링 관련은 App.js에서 하는게 맞다고 결론을 내렸기때문입니다.

import { addCartItem } from 'reducers/cart/cart.actions';

const Product = () => {
const { dispatch, isLoading, data, isError } = useReduxState('product');

Choose a reason for hiding this comment

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

잘만든 훅 열 컨테이너 안부럽다

Copy link
Author

@moonheekim0118 moonheekim0118 May 14, 2022

Choose a reason for hiding this comment

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

커스텀 훅 만들때마다 떠올리겠습니다,,

"잘만든 훅 열 컨테이너 안 부럽다!!"

const navigate = useNavigate();

useEffect(() => {
dispatch(getProductAsync(id));

Choose a reason for hiding this comment

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

dispatch, thunk, reduxState를 모두를 하나로 묶는 useProduct 훅으로 만들어서 일종의 Container 역할을 하는 비즈니스 훅을 만들어 관리하는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

좋은 아이디어 감사합니다! 생각해보니 dispatch , thunk 로직도 굳이 컴포넌트가 알 필요 없을 것 같아요!
이 부분 반영해서 useProduct, useProducts 훅 만들었습니다 : ) 컴포넌트에서 리덕스 로직이 확 빠지니까 훨씬 깔끔해졌네요 😍

dispatch(getProductAsync(id));
}, [id]);

const onClickCartButton = () => {

Choose a reason for hiding this comment

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

onClickCartButton 은 on-* 이 오는게 맞을까요?

Copy link
Author

Choose a reason for hiding this comment

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

늘 on-* 를 붙여야할지, handle-* 를 붙여야할지 고민이었는데요, 발리스타님 리뷰를 보고 저만의 기준을 정해보았습니다.

  • props로 받아오는 이벤트 핸들러의 이름의 경우 (즉, props 의 이름) -> on-* 을 붙인다.
    • 왜? 아래와 같이 컴포넌트에 '이벤트를 부착한다'라는 의미를 살리기 위해서!
const SomeComponent = ({onClick}) => {
    return <div onClick={onClick}/>
} 

// 아래와 같이 props 를 넘겨줄 수 있다!!

<SomeComponent onClick={/**/}/>
  • props 로 넘겨주는 혹은 직접 이벤트를 붙이는 핸들러 함수의 경우 -> handle-* 을 붙인다.
    • 왜? on~ 에 대한 핸들러라는 의미를 살리기 위해서!
const handleClick = () => { }; 
<SomeComponent onClick={handleClick}/>

Choose a reason for hiding this comment

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

너무 훌륭하네요.. 굿..

import useReduxState from 'hooks/useReduxState';

const ProductList = () => {
const { dispatch, isLoading, data, isError } = useReduxState('products');

Choose a reason for hiding this comment

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

여기도 useProducts 요런거 만드는게 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

위의 리뷰 에서 함께 반영했습니다 ❤️ 감사합니다 ~! 😄

Copy link
Author

@moonheekim0118 moonheekim0118 May 14, 2022

Choose a reason for hiding this comment

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

그런데 이렇게 hook으로 비즈니스 로직을 분리하다 보니 궁금한 점이 생겼습니다 👀
발리스타님은 컴포넌트에서 비즈니스 로직을 hook 으로 분리할 때 혹시 어떤 기준으로 하실까요? 페이지 내의 비즈니스 로직을 전부 하나의 hook에 두실까요, 아니면 각 연관된 비즈니스 로직별로 분리하실까요?
각각 장단점이 있을것같아요!

  • 페이지 내의 비즈니스 로직을 전부 하나의 hook에 둘 경우
    • (장점) 비즈니스 로직의 흐름을 파악하기 쉬위서 관리하기 쉬울 것 같아요.
    • (단점) 재사용이 되지 않고 중복되는 코드가 많아질 것 같아요.
  • 각 연관된 비즈니스 로직별로 분리 할 경우
    • (장점) 비즈니스 로직을 다른 컴포넌트나 페이지에서도 재사용 할 수 있을 것 같아요.
    • (단점) 비즈니스 로직의 흐름을 파악하기 조금 까다로울 것 같아요.

발리스타님이 비즈니스 로직을 분리하는 기준을 혹시 알려주실 수 있으실까요 ? ?

Choose a reason for hiding this comment

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

컴포넌트라서 다른 이야기긴 한데, https://github.com/Vallista/vallista-land/tree/main/packages/core/src/components/Checkbox 저는 모든 로직을 극단적으로 훅에만 넣고, 컴포넌트 내엔 넣지 않습니다 :)

일단 이런 기조가 있다는 배경을 생각해주시고, 설명드리겠습니다 :)

  • 먼저, 커스텀훅은 "공통화"가 되어 "공통적"으로 쓰이면 좋지만, 안쓰여도 괜찮습니다. 그저 비즈니스 로직등 여러 로직만 들어가있도록 생각하셔도 됩니다.
  • 프로젝트의 규모에 따라 유동적으로 선택하셔야 합니다~ 저는 그런데, 선택을 하기보다 두 방법을 전부 포용할 것 같아요~
  • 페이지 단위로 fit한 로직이 있을터이니, 해당 fit한 로직이 존재하는 1:1 대응되는 페이지용 비즈니스 훅
  • 공통 비즈니스로 떼어질 수 있는 비즈니스 로직 별 훅
  • 페이지 단위 비즈니스 훅은 여러 공통 비즈니스 로직 훅을 받아서 처리하고, 공통 비즈니스 로직 훅은 여러 API 로직, 상태, 등등의 공통 커스텀 훅 혹은 공통 상태들을 유기적으로 사용해 조합하는 형태가 될 것 입니다.
  • (컴포넌트) <- <- 컴포넌트 훅 <- 페이지 <- 페이지 단위 훅 <- 공통 비즈니스 훅 <- 비즈니스 훅 <- 상태, API 등..

Copy link
Author

Choose a reason for hiding this comment

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

와..정말 자세한 답변 감사드립니다..!! 👍 👍 👍 👍 이해가 쏙쏙 되었습니다..!!! ❤️

@moonheekim0118
Copy link
Author

moonheekim0118 commented May 14, 2022

안녕하세요 ✨발리스타님 ✨ 정말..정말 너무 너무 좋은 답변과 리뷰 감사드립니다..!! 현재 만드시는 제품을 통해서 설명해주신 덕분에 !! 정말 잘 와닿을 수 있었고, 간지러웠던 부분이 시원하게 긁혔어요 😃 ❤️ 그리고 두번째 질문 역시 발리스타님 말씀대로 store에 key - data 형식으로 가지고 있으면 문제 없을 것 같습니다 :)

또한 비즈니스 훅에 관련한 말씀도 큰 도움이 되었습니다.
늘 페이지와 비즈니스 로직을 꼭 분리해야할까? 어차피 해당 비즈니스 로직은 해당 페이지 내에서만 사용될텐데..? 라는 생각이 있었는데요! 발리스타님의 의견을듣고 페이지를 컴포넌트 개념으로 확장할 수 있다는 이점이 있다는 것을 깨달았습니다 : )

질문 주신것들 답변하고 고치면서 정말정말 많이 배웠고 재미있었어요 ㅎ-ㅎ 다시한번 감사드립니다.
말씀 주신 부분들 리팩토링/수정하였구, 질문들 각 코멘트에 남겨놓았습니다 ! !

리뷰 반영 사항 (해당 리뷰 코멘트 링크)

그 외에 리팩토링 한 부분 (커밋링크)

가 있구요, 그 외에는 스타일링 관련해서 폰트/CSS 조금 수정했습니다 ㅎㅅㅎ

덕분에 정말정말정말 많이 배워가고 즐겁게 학습했습니다 👍 👍
더 지적하실 부분이나 궁금한 점 / 질문하실 것들 있으시면 부탁드립니다 👯‍♀️ ❤️

Copy link

@Vallista Vallista left a comment

Choose a reason for hiding this comment

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

안녕하세요 호프님 :)

이번에 잘 적용해주신걸 보며, 호프님은 2단계도 잘 하실 것 같습니다 :)
질문 주신것도 넘 자세히 설명해주시고 찾아주시고 적어주셔서... 감동 (감동..)
제가 질문드린게 앞으로 많은 도움이 되었으면 좋겠습니다 :)

또한 코드도 잘 보았습니다!
문제 없이 2단계 진행하시면 될 것 같습니다.

시간나시면 코맨트 확인해주시고, 답변 달아주세용~

@Vallista Vallista merged commit 50dbe7d into woowacourse:moonheekim0118 May 15, 2022
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.

2 participants