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

4주차 1번째 과제 #60

Merged
merged 55 commits into from May 31, 2021
Merged

4주차 1번째 과제 #60

merged 55 commits into from May 31, 2021

Conversation

yujonglee
Copy link

1번째 과제 완료했습니다.

두 가지 의문이 있습니다.

  1. App.test.jsx에서 중복제거의 문제
    dispatchmocking하는 부분이 중복되어 리팩터링 책에 나온 예시대로 beforeEach로 중복을 제거해봤습니다. 그런데 let을 사용하지 않고 싶은데 어떻게 하는 것이 좋을까요? given으로 숨기는 것도 될 것은 같은데 별로 좋지는 않아보이고 뾰족한 방법이 떠오르지 않네요.
  let dispatch;
  beforeEach(() => {
    dispatch = jest.fn();
    useDispatch.mockImplementation(() => dispatch);
  });
  1. onChange 이벤트 객체의 문제

현재 updateTaskTitle을 아래와 같이 테스트하고 있습니다.

// 'codesoom'을 타이핑
    expect(dispatch).toHaveBeenLastCalledWith({
      type: 'UpdateTaskTitle',
      payload: {
        taskTitle: 'm',
      },
    });

그런데 처음부터 이렇게 하려고 했던 것은 아니고, 아래와 같이 하려고 했는데 테스트가 통과를 못해서 바꿔준 것입니다.

// 'codesoom'을 타이핑
    expect(dispatch).toHaveBeenLastCalledWith({
      type: 'UpdateTaskTitle',
      payload: {
        taskTitle: 'codesoom',
      },
    });

onChange에서 받는 이벤트 객체에서 event.target.value를 해서 taskTitle을 업데이트 해주니까 c, co, cod, code, codes, ... codesoom 이렇게 찍혀야 할 것 같은데(콘솔로 찍어봐도 그렇습니다.) 테스트할 때 오류 메시지는 c, o, d, e, s, o, o, m 이런 순서로 dispatchcall된다고 하네요. 제가 무엇을 잘못 생각하고 있는 것일까요?

@Kihyun92 Kihyun92 self-requested a review May 24, 2021 13:39
Copy link
Collaborator

@Kihyun92 Kihyun92 left a comment

Choose a reason for hiding this comment

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

오,, 벌써 이 만큼을 진행하셨군요 👍🏼 👍🏼 리덕스에 익숙하신 것 같네요 ㅎㅎ 그리고 커밋을 보니 TDD cycle에 맞추려고 노력해주신 것 같습니다~

Redux를 도입한 지금이 App 컴포넌트의 관심사가 무엇인지 다시 한번 고민해볼 때인 것 같습니다.
이전 과제에서 유종님께서 Container/Presentation 모델을 적용해주셨던 기억이 나요. 컴포넌트들의 관심사에 따라서 기존 구조들을 한번 변경해보시면 어떨까요?

추가로 질문에 대한 답변은 해당 코드에 남겼습니다! 확인 부탁드려요~

src/App.test.jsx Outdated Show resolved Hide resolved
src/App.test.jsx Outdated Show resolved Hide resolved
src/App.test.jsx Outdated Show resolved Hide resolved
src/actions.js Outdated Show resolved Hide resolved
src/reducer.js Outdated Show resolved Hide resolved
src/reducer.test.js Outdated Show resolved Hide resolved
src/App.test.jsx Outdated Show resolved Hide resolved
@yujonglee
Copy link
Author

yujonglee commented May 25, 2021

자잘한 수정 이후 Container/Presentation모델을 도입했습니다.
이미 다 짜놓은 상태에서 도입하는 작업이 간단하지는 않았는데, 작은 단위로 나누어서 리듬을 느끼면서 진행하니 점점 진전이 되면서 완성이 되었네요.

제가 전에 Container/Presentation 모델을 몇번 써보기도 하고, 지금도 도입해보면서 이렇게 UI쪽이랑 상태관리/비즈니스 로직을 분리하는 것은 좋다고 느껴지는데요.

이전에 리액트를 공부한지 얼마 되지 않았을 때(지금도 얼마 되지는 않았지만) 리액트 훅이 생긴 이후로는 굳이 컴포넌트를 나눌 필요없다는 글을 접했었는데(예시) 이제 다시 기억이 나네요.

코드를 다 수정한 뒤에 생각이 나서 이제야 다시 찾아보고 생각해보려고 하는데, 혹시 이런 주제에 관한 트레이너님의 생각이 궁금합니다.

@Kihyun92
Copy link
Collaborator

자잘한 수정 이후 Container/Presentation모델을 도입했습니다.
이미 다 짜놓은 상태에서 도입하는 작업이 간단하지는 않았는데, 작은 단위로 나누어서 리듬을 느끼면서 진행하니 점점 진전이 되면서 완성이 되었네요.

제가 전에 Container/Presentation 모델을 몇번 써보기도 하고, 지금도 도입해보면서 이렇게 UI쪽이랑 상태관리/비즈니스 로직을 분리하는 것은 좋다고 느껴지는데요.

이전에 리액트를 공부한지 얼마 되지 않았을 때(지금도 얼마 되지는 않았지만) 리액트 훅이 생긴 이후로는 굳이 컴포넌트를 나눌 필요없다는 글을 접했었는데(예시) 이제 다시 기억이 나네요.

코드를 다 수정한 뒤에 생각이 나서 이제야 다시 찾아보고 생각해보려고 하는데, 혹시 이런 주제에 관한 트레이너님의 생각이 궁금합니다.

우선 좋은 글 공유해주셔서 감사합니다 👍🏼

관심사를 분리한다는 점에서 굉장히 유용한 패턴이라고 생각해요. 문서에도 나오지만, 자연스럽게 Presentation/Container로 분리하는게 좋죠.

Dan의 말처럼 동일하게 리액트의 커스텀 훅을 사용해서 관심사를 분리해줄 수 있습니다. 지금은 관심사의 분리가 어떤 관점에서 이루어지는지를 고민해보는 단계라고 생각하시면 좋을 것 같습니다. ㅎㅎ

추후에 과제에서 커스텀 훅을 직접 다뤄 보셔도 좋을 것 같네요~ 😊

관련해서 코드숨 슬랙 채널에 공유해서 의논해봐도 좋을 것 같군요! 앞으로도 이런 의논점이 있다면 공유해주셔요 ㅎㅎ

Copy link
Collaborator

@Kihyun92 Kihyun92 left a comment

Choose a reason for hiding this comment

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

리뷰 드린 것보다 더 잘 반영해주신 것 같군요 👍🏼

src/App.jsx Outdated Show resolved Hide resolved
src/presentational/Input.test.jsx Outdated Show resolved Hide resolved
Copy link
Collaborator

@Kihyun92 Kihyun92 left a comment

Choose a reason for hiding this comment

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

더 드릴 피드백이 없어 보이네요 👍🏼

@Kihyun92 Kihyun92 merged commit 6863582 into CodeSoom:yujong-lee May 31, 2021
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