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

[2단계 - 장바구니 협업] 밧드(감우영) 미션 제출합니다. #61

Merged
merged 58 commits into from
Jun 19, 2022

Conversation

kamwoo
Copy link

@kamwoo kamwoo commented Jun 9, 2022

안녕하세요! 아사 아래 데모페이지 링크입니다.
데모페이지

1단계 리뷰반영

말씀드리지 못한 부분이 있었는데, 협업 미션을 들어가면서 새로 만난 페어 둘중 하나의 코드를 선택하여 사용해야했습니다. 1단계 피드백의 대부분이 제 코드가 아니어서 피드백 해주신 부분 이외에 추가로 리펙토링을 했지만 세세한 부분까지는 파악을 못해서 많이 수정하지는 못했습니다.😭
백엔드와 api와 연동을 하면서 리덕스 관련한 코드를 추가로 작성했고 렌더링 부분, 동작하지 않는 부분들이 아직 있어서 추가로 수정하겠습니다.

변경사항

명세를 작성하면서 request, response에 따라 객체 속성 네이밍, props가 변경되었습니다. 그리고 스낵바, 로그인 상태 체크 등이 추가되었습니다.

질문사항

로그인, 회원 가입같이 성공하면 다른 페이지로 이동시킬 때, 지금은 리덕스 상태에서 loading나 data를 useEffect dependencies에 넣고 userData가 있다면 이동시키는 방식을 사용하고 있습니다. 불안정하고 제대로된 방법이 아니라고 생각하는데, 지금과 같은 상황에서 가장 좋은 방법이 무엇이 있을 지 궁금합니다.

에러사항

상품 리스트에서 장바구니 버튼 연속으로 누를 때마다 장바구니 추가, 장바구니 페이지에서 수량변경
-> 리펙토링하면서 response key값에서 id, productId가 섞였다고 생각해서 찾고 있습니다.

@pocojang pocojang marked this pull request as draft June 9, 2022 08:47
@kamwoo kamwoo marked this pull request as ready for review June 9, 2022 10:44
@pocojang pocojang requested a review from EungyuCho June 10, 2022 01:06
Copy link

@EungyuCho EungyuCho left a comment

Choose a reason for hiding this comment

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

안녕하세요 밧드 :)
잘 지내셨죠?

다른 크루 코드로 작업하는게 쉽지 않으셨을 것 같아요. 거기다가 모두 typescript로 되어있네요..! 😇
현업에 가셔서도 비슷한 개발환경일 확률이 높아보이는데 좋은 기회라고 생각이 들었어요 ㅎㅎ

혹시나 디버깅을 하시다가 다시 만드는게 더 빠를 것 같다고 생각이 드신다면 갈아엎으시는것도 하나의 방법이니 긍정적으로 검토해보셔도 좋을거에요~!

고민

로그인, 회원 가입같이 성공하면 다른 페이지로 이동시킬 때, 지금은 리덕스 상태에서 loading나 data를 useEffect dependencies에 넣고 userData가 있다면 이동시키는 방식을 사용하고 있습니다. 불안정하고 제대로된 방법이 아니라고 생각하는데, 지금과 같은 상황에서 가장 좋은 방법이 무엇이 있을 지 궁금합니다.

별도로 페이지 내부에서 useEffect로 잡는 것 보단 handleSubmit 함수에서 signin 등 thunk dispatchf를 하는 라인 바로 밑에서 원하시는 페이지로 navigate를 하면 되지 않을까요?
만약 가입이나 로그인이 실패했다면 에러 핸들링으로 처리하면 되는 부분인 것 같구요 🤔
혹시 useEffect로 처리해야겠다고 생각하신 이유가 있을까요?

추가로! 본문에서 말씀해주신대로 아래처럼 동작하지 않는 케이스를 커버해보면 좋을 것 같아요. (리마인드 차원 기록)

  • 로그인 후 장바구니에 상품들을 담고 로그아웃 후 다시 장바구니로 돌아왔는데 새로고침을 하기 전까지 이전에 담았던 상품들이 비 록읜 상태인데 남아있어요.
  • 장바구니에서 데이터들이 서버에 요청은 보내는데 클라이언트 데이터가 인터렉션한 대로 동작을 하지 않아요. (상품 체크, 수량 증가 등)
  • 로더 위치가..!

image

미션 진행하시느라 고생하셨어요 밧드!
방학 잘 보내세요 🙂

};
}, []);

return text && <Styled.Box key={Math.random()}>{text}</Styled.Box>;

Choose a reason for hiding this comment

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

key는 왜 필요한걸까요? 😮

Copy link
Author

@kamwoo kamwoo Jun 16, 2022

Choose a reason for hiding this comment

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

장바구니 추가 버튼을 클릭했을 때마다 스낵바가 새로 mount되었으면 했습니다. 리덕스 메세지 상태를 객체로 만들어서 useEffect에 timeout을 새로 실행시키고, 매번 key를 다르게 줘서 리렌더링되도록 했습니다.

Comment on lines +25 to +33
const createReducer = <T, A>(initialState: T, handlers: A) => {
return (state = initialState, action) => {
if (handlers.hasOwnProperty(action.type)) {
return handlers[action.type](state, action);
} else {
return state;
}
};
};

Choose a reason for hiding this comment

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

👍 👍

Comment on lines 114 to 117
} catch (error) {
dispatch(userAction.autoSignIn.failure(error.response.data.errorMessage));
alert(error.response.data.errorMessage);
}

Choose a reason for hiding this comment

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

alert로 메세지를 보여주는게 좋은 유저경험은 아니기에 혹시 여유가 되신다면 바꿔보셔도 좋을 것 같아요~

Copy link
Author

Choose a reason for hiding this comment

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

넵 전달해야되는 메세지는 스낵바로 대체하겠습니다. 👍

dispatch({ type: UserActionType.AUTO_SIGN_IN_FAILURE, payload: e.message });
alert(e.response.data.errorMessage);
dispatch(userAction.autoSignIn.success(response.data));
} catch (error) {

Choose a reason for hiding this comment

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

토큰을 보냈을 때 인증이 만료되었을 때 다시 로그인을 하는방향으로 유도하는 로직이 없는 것 같은데 추가해보면 어떨까요? 👀

Comment on lines +34 to +42
const response = await axios({
method: 'patch',
url: `${BASE_URL}/cart`,
headers: {
'Content-Type': 'application/json',
Authorization: `Bearer ${token}`,
},
data: { cartItems: cartItemList },
});

Choose a reason for hiding this comment

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

api를 요청하는 별도의 모듈을 만들면 어떨까요?
중복되는 구현도 줄일 수 있고 일관된 예외처리도 수월할거라고 생각이 들어요~

Copy link
Author

@kamwoo kamwoo Jun 16, 2022

Choose a reason for hiding this comment

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

이게 관련된 얘기인것 같아서, 1단계에서 피드백 주신 Suspense를 적용하기 위해서 pending 상태에 Promise를 throw하는 모듈을 만들어야 되겠다고 생각했고, fetching하는 모듈을 따로 만들어서 데이터를 필요로하는 컴포넌트에서 request를 하고 정상적인 응답만 dispatch하고, suspense로 로딩을 띄우고, ErrorBoundary로 에러관련 컴포넌트를 띄우면
전역 상태에 pending, success, failure이 필요없을 것 같다고 생각했습니다.
말해주신대로 api요청하는 모듈을 만들면서 같이 적용해보겠습니다.

price: number;
}

export interface CartItem {
id: number;
type?: 'CartItem';

Choose a reason for hiding this comment

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

타입에 CartItem이 있는 이유와 해당 속성이 optional인 이유가 궁금해요 👀

Copy link
Author

Choose a reason for hiding this comment

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

Promise상태에 따라 액션 생성 함수를 만드는 generateAsyncActionGroup함수에서 action type을 string으로 지정해서 reducer에서 narrowing이 안됐고, type필드로 좁힐려고 했지만 이것도 잘안됐습니다. 해당하는 type은 필요없어서 지우고 action type을 수정하도록 해보겠습니다!

Comment on lines 93 to 94
dispatch(userAction.deleteUser.failure(error.response.data.errorMessage));
alert(error.response.data.errorMessage);

Choose a reason for hiding this comment

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

alert를 넣어주신게 의도하신게 맞을까요?? 👀

Copy link
Author

@kamwoo kamwoo Jun 16, 2022

Choose a reason for hiding this comment

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

alert를 띄우는 모든 로직이 api연동을 하면서 쉽게 확인할려고 넣었습니다. 위에서 말해주신대로 스낵바나 다른 방법으로 전달하도록 수정했습니다.

kamwoo added 28 commits June 16, 2022 13:19
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.

리뷰 기간이 마감되어 머지합니다

@pocojang pocojang merged commit 9f52fe5 into woowacourse:kamwoo Jun 19, 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.

None yet

3 participants