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주차 코드리뷰 요청 #146

Open
wants to merge 4 commits into
base: ctaaag
Choose a base branch
from
Open

4주차 코드리뷰 요청 #146

wants to merge 4 commits into from

Conversation

ctaaag
Copy link

@ctaaag ctaaag commented May 23, 2023

아직 2강까지 밖에 못들어서 해당 부분까지 우선 PR 보냅니다 :)
폴더 분리 곧 해서 커밋하겠습니다!

}

function handleClickAddTask() {
dispatch((addTask()));
Copy link
Contributor

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.

앗..!! 수정하겠습니다

src/reducer.js Outdated
};

export default function reducer(state = initialState, action) {
if (action.type === 'updateTaskTitle') {
Copy link
Contributor

Choose a reason for hiding this comment

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

if문이나 스위치문 없이 코드를 작성할 수 있을까요?

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 34 to 44
describe('task를 삭제한다', () => {
it('task가 없어진다', () => {
const prevState = {
newId: 101,
taskTitle: '',
tasks: [{ id: 100, taskTitle: 'something' }],
};
const state = reducer(prevState, deleteTask(100));
expect(state.tasks).toHaveLength(0);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

reducer는 이전 상태와 액션을 받고 새로운 상태를 반환하는 함수입니다. 그러면 없어진다라고 표현하는 것보다는 다르게 표현할 수 있을거예요

Copy link
Author

Choose a reason for hiding this comment

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

호오.. 고민해보고 커밋보내겠습니당!

@ctaaag
Copy link
Author

ctaaag commented May 25, 2023

[변경사항]

  1. redux-actions를 활용해 if문 없이 사용하기
    이번에 redux-actions를 처음 활용 해보면서 handleActions, createAction 메서드를 활용해봤습니다
    쓰다 보니까 toolkit으로 createAction과 createReducer를 활용하고 싶은 마음이 굴뚝 같았지만
    toolkit을 안쓰고 구현을 하는게 취지에 맞을 것 같아 사용하지 않았습니다..!! 🥲

  2. reducer test 문구 변경
    업어진다는 표현이 새로운 상태값을 반환한다는 것을 나타내기엔 적절하지 않은 것 같아서 변경했습니다.

[질문]

  1. Jest에서 redux-action module을 찾지 못하는 에러
    제스트로 테스팅할 때 redux-actions 모듈을 찾지 못하는 에러가 발생하고 있습니다..!!
    jest.config.js에서 root 경로 설정을 추가하거나, node_modules 전부 삭제후 재설치해도 테스트에서 에러가 발생하네요ㅜㅜ
    로컬호스트로 실행 후 웹환경에서는 에러없이 잘 됩니다!
    혹시 어떻게 해결할 수 있을까요?
  • 에러 메세지
스크린샷 2023-05-25 오후 8 08 33
  • package.json
스크린샷 2023-05-25 오후 8 09 33
  • jest.config.js
스크린샷 2023-05-25 오후 8 09 39


const initialState = { newId: 100, taskTitle: '', tasks: [] };

const reducer = handleActions({
Copy link
Contributor

Choose a reason for hiding this comment

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

reduc-actions를 사용하셨군요. redux-actions를 의도한 것은 아니었지만, 이런식으로 객체를 사용해서 if문이나 switch문 없이 구현하는 것을 의도했었어요 ㅎㅎ

Copy link
Author

Choose a reason for hiding this comment

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

헛 그랬군요..!! redux-actions를 활용하지 않으면 객체형태로 하는게 어렵지 않을까 생각했었는데 아니였군요!
과제 2에서는 redux-actions를 쓰지 않고 구현해보겠습니다 :)

@hannut91
Copy link
Contributor

redux-action module을 찾지 못하는 문제는 여러가지 시도를 해봤는데 아직 해결은 못했어요.

jest는 CommonJS모듈 방식을 사용하는데, 이 방식은 외부 모듈의 경우 /node_modules/redux-actions로 일단 찾아갑니다. 그다음에 package.json내용을 기반으로 모듈을 찾는데, main이라는 속성이 있으면 이 파일로부터 모듈을 찾는데 만약 없으면 그냥 index.js파일로 찾습니다. 없는 경우 없다고 에러를 발생시켜요.

근데 redux-actions의 package.json을 보면 exports가 있습니다. jest에서 이게 제대로 동작하려면 CommonJS모듈로 export되야되는데 redux-actions는 그렇지 않은 것 같더라구요. 그래서 못찾는게 아닌가 싶습니다.

그래서 exports를 지우고 redux-actions.js를 복사해서 index.js로 하면 모듈을 찾는 문제는 해결되긴하는데, 이제 또 다른 문제가 발생합니다. ESModule도 작성되어 있어서 jest에서는 문제가 또 발생해요... 아직 이 문제를 해결하지 못했어요

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