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

전남대FE_김시현_5주차 과제 #251

Merged

Conversation

sihyonn
Copy link

@sihyonn sihyonn commented Jul 28, 2023

안녕하세요 멘토님! 벌써 5주차 과제를 제출하네요🫢
매주 리뷰 정말 감사드리며 이번주차 과제도 잘 부탁드립니다.
테스트는 처음해보는거라 이렇게 접근하는게 맞는건지 잘 모르겠습니다.!
전체적으로 부족한 점과 개선해야하는 부분 새겨듣겠습니다.
그리고 그동안 부족한 개념공부 채우느라 못했던 리팩토링도 차근차근 진행해보겠습니다.
감사합니다!!


주문결제 페이지 개발

5주차과제


테스트

테스팅은 찾아보니 react testing library와 jest로 많이 하는 것 같아서 시도해보았는데
아래와 같은 오류가 나는데 Jest가 기본적으로 ECMAScript Modules를 지원하지 않기 때문에 발생한다고 알게되었습니다.
그래서 ECMAScript Modules를 Jest에서 직접 지원하도록 설정을 해보았는데 이게 아닌(?) 것 같습니다..
테스트를 이렇게 일단 jest로 테스트파일을 만들어서 테스트코드를 작성하는게 올바른지,
ECMA에 관한 부분은 어떻게 처리해야하는지 여쭙니다.

C667A13E-168C-4A8D-B99C-16A30B8EFAE6_1_105_c

@Junkim93 Junkim93 self-assigned this Jul 29, 2023
Comment on lines +18 to +20
const agreeAllRef = useRef(null);
const agreePaymentRef = useRef(null);
const agreePolicyRef = useRef(null);
Copy link

Choose a reason for hiding this comment

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

이 ref 변수는 현재 존재하는 이유가 없는 것 같은데 지워주는게 어떨까요?


const handleClickOrder = () => {
// 동의 하나라도 안이루어진 경우
if (agreePayment === false || agreePolicy === false) {
Copy link

Choose a reason for hiding this comment

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

Suggested change
if (agreePayment === false || agreePolicy === false) {
if (!agreePayment || !agreePolicy) {

이렇게 바꿔줄 수 있겠습니다.

mutate();
};

const handleAgreeAll = (e) => {
Copy link

Choose a reason for hiding this comment

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

여기도 변수명에서 click 이벤트에 대한 핸들러 인지 알 수 있게 해주면 좋을 것 같아요.

};

// OrderItems
const OrderItems = () => {
Copy link

Choose a reason for hiding this comment

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

이 부분은 다른분들이랑 같은 의견 드리겠습니다.

이 컴포넌트는 별도로 분리해주시면 가독성 측면에서 더 좋겠습니다.
그리고 가능하면 루프를 두 번 돌기보다는 flatMap 같은 메서드를 활용해서
코드의 depth가 깊어지지 않도록 하는게, 코드의 가독성을 유지하는데 좋을 것 같습니다.

현업에서는 소나큐브 같은 도구를 활용해서 코드의 복잡도를 분석하고,
코드의 복잡도가 너무 높으면 PR 자체가 머지되지 못하도록 설정해놓는 경우가 많습니다.
이렇게 depth가 깊어지는 코드가 보통 그 대상이 됩니다.

let renderComponent = [];
이런 부분도 사실 map 메서드를 활용해서 충분히 해결할 수 있는 부분이라,
변수를 하나 더 생성해서 불필요한 코드를 늘리기보다는,
가능하면 변수 생성 없이 함수형 메서드로 해결해보시는 습관을 들이시면 좋겠습니다~!

const navigate = useNavigate();
useEffect(() => {
if (localStorage.getItem("token") === null) {
alert("로그인이 필요한 서비스입니다🤗");
Copy link

Choose a reason for hiding this comment

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

Outlet에 들어갈 하위 라우트 컴포넌트들에서 API가 먼저 호출되면 어떻게 될까요?
/order 페이지에 한번 들어가보시고,
/cart 페이지에서의 상황과 차이점을 비교해보신뒤
수정해보시면 좋겠습니다~!

@@ -34,7 +34,6 @@ const CartPage = () => {
return (
<div className="cartpage">
<Suspense fallback={<Loader />}>
{/* <CartList data={cartItems} /> */}
{cartItems.length !== 0 && <CartList data={cartItems} />}
Copy link

Choose a reason for hiding this comment

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

여기서 조건식을 걸어줄 수도 있지만, CartList에서 기본 데이터를 의도적으로 빈 배열로 만들어줄 수도 있겠습니다.
CartList 에서 데이터가 없어서 그릴 수 없는 부분은 렌더링 되지 않고, 나머지 요소만 렌더링되도록요.

@Junkim93
Copy link

Junkim93 commented Aug 2, 2023

안녕하세요 시현님,
create-react-app으로 프로젝트를 생성한 경우 기본적인 Jest 설정이 이미 내장되어 있습니다.
그런데 이 설정에 node_modules에 대한 처리가 안되어있어서 해당 에러가 발생하고 있는 것인데요.
이 이슈 관련해서는 아래 링크를 참고해보시면 좋겠습니다.
facebook/create-react-app#9938

그런데 이렇게 실행한다고 하더라도,
현재 시현님이 테스트하려는 컴포넌트는 의존성이 너무 많습니다.
이런 부분을 모킹하는 등 대체하는 처리가 필요한데요.

가장 간단하게 시작하는 방법은
특정 라이브러리에 의존하지 않는 순수한 로직의 input과 output만 테스트 하는 것입니다.
그런데 이것도 되게 하려면 로직의 분리가 잘 되어있어야 합니다 😅

우선은 Cypress나 puppeteer 같은 E2E 테스트 도구를 활용해보시면 좋겠네요.
이 부분은 지금 단계에서 설명하기에는 복잡한 감이 있어서 제가 채널에 따로 공유를 드려볼게요.

@Junkim93 Junkim93 merged commit bcd4786 into Kakao-tech-campus-FE:feat-KimSiHyeon Aug 2, 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.

2 participants