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

[ Init/msw 39 ] msw + vitest 환경 구축 및 next 적용 #40

Merged
merged 9 commits into from
Jun 10, 2024

Conversation

Happhee
Copy link
Collaborator

@Happhee Happhee commented Jun 10, 2024

🔥 Related Issues

resolve #39
close #39

💜 작업 내용

  • next환경에서 msw 사용할수있도록 구현

✅ PR Point

mocks

  • response 👉 목데이터 정의
  • browser, server 👉 next 구동환경 나눠서 적용
  • MSWProviders.tsx 👉 모킹 활성화안되어있을때 체크해서 렌더링 하도록 만든 컴포넌트 , ssr시 예외처리를 위해.

next.config.mjs

config설정을 통해msw 모듈을 server, browser로 나눠서 읽을수있도록 수정

if (context.isServer) {
      if (Array.isArray(config.resolve.alias)) {
        config.resolve.alias.push({ name: "msw/browser", alias: false });
      } else {
        config.resolve.alias["msw/browser"] = false;
      }
    } else {
      if (Array.isArray(config.resolve.alias)) {
        config.resolve.alias.push({ name: "msw/node", alias: false });
      } else {
        config.resolve.alias["msw/node"] = false;
      }
    }

😡 Trouble Shooting

아래 사진처럼 mocking 연결이후에 한번 404에러뜨는데..오류를 같이 찾아봤으면 좋겟어용..!
일단 잘 불러와지긴합니다..!

👀 스크린샷 / GIF / 링크

image

📚 Reference

-Next.js + msw 도입기

@Happhee Happhee linked an issue Jun 10, 2024 that may be closed by this pull request
1 task
} else {
config.resolve.alias["msw/node"] = false;
}
}
return config;
},
};

Choose a reason for hiding this comment

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

다음은 이 코드 패치에 대한 간략한 리뷰입니다:

  1. 가능성 있는 버그: webpack 설정에서 config.resolve.alias를 배열로 다루는 로직이 존재합니다. 그러나 일반적으로 Webpack configuration의 resolve.alias는 객체이므로, 배열로 처리하기 위한 조건문은 부적절해 보입니다.

  2. 기능 개선 제안: 눈에 띄는 중복 코드가 있습니다. context.isServer의 들여쓰기 하에서 msw/browsermsw/node를 구분하는 것만 다르고 나머지 코드는 같습니다. 이런 중복은 디자인 패턴으로 분리하여 간결하게 만들 수 있습니다.

예를 들어:

const target = context.isServer ? "msw/browser" : "msw/node";
config.resolve.alias[target] = false;
  1. 확인 필요 사항: alias: false 업데이트를 통해 얻고자 하는 효과를 주의 깊게 검토하는 것이 중요합니다. 일반적인 webpack alias 설정에서는 이 이름을 원래 모듈 경로에 매핑하는데, 이 경우 false로 설정하면 모듈 경로를 비활성화하는 것으로 해석됩니다. 목적이 이것이 아니라면 예상치 못한 동작 오류를 발생시킬 수 있습니다.

data: DataType;
};
export interface ApiResponse<DataType> {
data: Response<DataType>;
error?: {
code: number;
detail: string;

Choose a reason for hiding this comment

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

코드 패치에 대한 리뷰는 다음과 같습니다:

  1. 새로운 Response<DataType> 타입은 신중하게 고려해야 합니다. 이전 버전에서 data와 meta는 별도로 분리되어 있었습니다만, 이제는 'data'라는 프로퍼티 안으로 'message' 정보가 들어갔습니다. 이것이 원하던 바인지 확인이 필요합니다.

  2. ApiResponse의 'data' 필드는 이제 'Response' 객체를 컨테이닝 하고 있습니다. 이는 API 응답의 데이터 구조를 변경합니다. 따라서 이 변경이 기존 코드에서 접근하는 모든 부분에 영향을 미칠 수 있습니다.

  3. Method 타입의 사용이 누락되어 있으며 이는 나중에 사용할 계획이면 상관 없지만 그렇지 않다면 삭제하는 것이 코드 정리에 도움이 됩니다.

  4. error? 프로퍼티는 선택적입니다. 제대로 작동하기 위해서는 처리 단계에서 error를 체크해야 합니다.

개선사항은 개발자와 관련 팀간에 종속적입니다. 예를들어, API 리스폰스 데이터 형식을 변경하면 클라이언트 코드에 영향을 줄 수 있으니 이에 대한 충분한 소통과 계획이 준비되어야합니다.

<Suspense>{children}</Suspense>
<MSWProviders>
<Suspense>{children}</Suspense>
</MSWProviders>
</body>
</html>
</QueryClientProviders>

Choose a reason for hiding this comment

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

다음과 같은 점들을 검토하는 것이 좋을 것 같습니다:

  1. 코드를 확인했을 때, MSWProviders가 성공적으로 가져와졌는지 검증하세요. 또한 해당 모듈이 어떠한 기능을 담당하는지 확인하기 위해 해당 문서를 참조하세요.

  2. Suspense 컴포넌트 내에서 발생할 수 있는 오류를 적절하게 처리하고 있지 않은 것 같습니다. 가능한 이슈에 대해 예외 처리를 추가하는 것이 좋습니다.

  3. 과도하게 의존어를 사용하는 클래스 이름 (예: "min-h-[100dvh] w-full max-w-[480px] overscroll-y-none")은 재사용성에 문제가 될 수 있으므로 개선할 필요가 있습니다. CSS-in-JS 도구 (예: Styled-components 또는 Emotion)를 사용하여 컴포넌트 스타일링을 관리하는 것을 고려해보세요.

  4. 작성된 테스트 코드의 유무와 테스트에 포함된 모든 시나리오를주의 깊게 검토합니다. 코드 변경으로 인해 기존 기능에 영향을 주는 경우가 있는지 확인하는 것이 중요합니다.

  5. 마지막으로, 좋은 코드는 자체 설명력이 있어야 합니다. 변수, 함수 및 컴포넌트 이름이 명확한지 확인하세요. 또한 필요에 따라 적절한 주석을 추가합니다.

if (!mswReady) return null;

return <>{children}</>;
}

Choose a reason for hiding this comment

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

제공된 코드 패치를 검토하였습니다. 다음은 제가 이 코드에 대해 가진 생각입니다:

  1. "useState"에서 초기값을 주는 부분이 조금 복잡해 보입니다. 가독성을 위해 상단에 상태를 선언하는 것을 추천드립니다.

    const initialMockState = !isMockingMode;
    const [mswReady, setMSWReady] = useState(initialMockState);
    
  2. useEffect 안에서 init 변수 설정 후 해당 함수를 호출하는 것도 복잡해보입니다. 로직을 간단하게 만들기 위해 바로 실행 가능 하게 비동기 함수를 '즉시 실행' 형태로 변경할 수 있는데 이 방식을 고려해 볼만 합니다.

     useEffect(() => {
       (async function() {
         if (!mswReady && isMockingMode) {
           // 모의응답 초기화
           const { initMocks } = await import("@mocks/index");
           await initMocks();
           setMSWReady(true);
         }
       })();
     }, [mswReady]);
    
  3. 최상위 컴포넌트는 적절한 메모리 사용을 위해 React.memo로 감싸주는 것이 좋습니다.

  4. 코드全体적으로 더 명확한 주석이 필요해보입니다.

  5. 에러 처리 로직이 없어보입니다. 'next.js' 상에서 unhandled promise rejections and async function exceptions는 silent failures로 이어집니다. 따라서, initMocks 호출에서 예외가 발생할 경우 그것을 캐치하고 적절히 처리하는 것이 중요합니다.

  6. 또한, useEffect 내부에 있는 비동기 함수를 실행할 때, 해당 컴포넌트가 언마운트 되었을 때 상태를 설정하지 않도록 확인하는 로직이 필요합니다. 아니면, 'Can't perform a React state update on an unmounted component'와 같은 에러가 발생할 수 있습니다.

코드 품질 개선은 지속적인 과정이므로, 이상의 사항들은 단지 제안일 뿐입니다. 코드 요구사항과 프로젝트의 전체적인 맥락에 따라 결정해야 합니다.


import { handlers } from "./handlers";

export const worker = setupWorker(...handlers);

Choose a reason for hiding this comment

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

이 코드 조각은 기본적으로 깔끔하고 명확하게 쓰여졌습니다. 그러나 여전히 몇 가지를 검토할 만한 내용이 있습니다.

  1. "msw/browser"와 "./handlers"에서 가져온 모듈이 실제로 설치되어 있는지 확인하세요. 이 패키지가 없다면 코드는 실행되지 않을 것입니다.

  2. handlers가 배열인지 확실히 확인하십시오. setupWorker(...handlers) 호출에서 스프레드 연산자(...)를 사용하고 있기 때문에, handlers가 배열이 아니라면 런타임 오류를 발생시킬 가능성이 있습니다.

  3. 마지막으로, worker 변수를 다른 곳에서 사용할 계획이라면 이 값을 어디서나 접근할 수 있도록 해야 합니다. 이는 모듈의 성격에 따라 달라집니다만, 흔히는 이 값을 사용할 수 있도록 내보내거나 (export) 전역 범위로 선언해야 할 수도 있습니다.

위 사항들을 검토함으로써 이 코드 조각은 좀 더 안정적이며, 유지 관리하기 용이해질 것입니다.

return HttpResponse.json(response[apiRoutes.problems]);
});

export const handlers = [quizHandler, tagsHandler, problemsHandler];

Choose a reason for hiding this comment

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

이 코드를 검토하면서 일부 버그 위험이나 개선 제안을 발견했습니다.

  1. problemsHandler에서, 쿼리 매개변수로 인해 articleId를 얻고 있지만, 이 값에 대한 처리가 없습니다. 만약 'articleId'에 따라 다른 결과를 반환해야 한다면, 해당 로직이 보이지 않습니다. 아마도 response[apiRoutes.problems]를 호출할 때 'articleId'를 사용하려고 했던 것일 수도 있습니다.

  2. 에러 처리 시 상태 코드 404를 반환하는 것은 좋지만, 오류 메시지를 함께 전달해주는 것이 더 좋을 것입니다. 이를 통해 클라이언트는 정확히 어떤 문제가 발생했는지 알 수 있습니다. 예를 들어, new HttpResponse({message: "Article ID not found"}, { status: 404 })와 같이 사용할 수 있습니다.

  3. http.get을 사용하여 여러 API 경로에 EventHandler를 함께 연결하고 있습니다. 각각의 독립적인 기능을 가진 모듈로 분리하여 코드의 가독성을 향상시키는 것을 고려해보세요.

  4. 마지막으로, 테스트용 응답 데이터(response)는 파일 내부에 정의되어 있지 않으므로 외부에서 불러오는 것이라면 적절한 에러 핸들링을 해주는 것이 중요합니다. 이 데이터가 중간에 변경되거나 사라진다면 여기서 문제가 발생할 수 있습니다.

코드 리뷰는 여러 가지 각도에서 수행될 수 있는 작업이므로 더욱 디테일한 차원에서의 피드백을 원한다면 추가 정보 제공해주시면 감사하겠습니다.

const { worker } = await import("./browser");
worker.start({ onUnhandledRequest: "bypass" }); // 처리되지 않은 요청이라도 통과시키도록
}
}

Choose a reason for hiding this comment

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

이 코드 패치를 검토하는 데 있어서 발견된 몇 가지 사항들은 다음과 같습니다:

  1. 에러 처리: server.listenworker.start 에서 발생할 수 있는 에러에 대한 처리가 없습니다. 필요한 경우 try/catch 구문을 사용하여 예외를 처리해야 합니다.
  2. 주석: 어떤 경우에도 처리되지 않는 요청을 통과시키는 디자인 결정에 대한 설명이 추가적으로 필요합니다. 이는 코드의 유지 보수를 할 때 맥락을 이해하는 데 도움이 됩니다.
  3. 가독성: server 또는 worker와 같은 module import를 함수 상단에 위치시키는 것이 일반적입니다. 그러나 이는 동적으로 import를 하려는 의도라면 필요하지 않습니다.
  4. typeof window === "undefined" 확인은 브라우저 환경에서 실행되는 지 서버 환경에서 실행되는지를 확인하는 일반적인 방법입니다. 당신의 코드가 이 두 환경을 모두 지원한다고 가정하면 이는 필요한 점검일 것입니다.

기능적으로, 이 코드는 서버와 브라우저 환경에서 동작해야 하는 경우에 대한 개별 mocking 시스템을 초기화하는 역할을 하는 것처럼 보입니다. 만약 이 코드가 실행 환경이 분명한 컨텍스트에서 사용된다면, "isServer" 플래그의 필요성을 재검토하는 것이 좋을 수도 있습니다.

전반적으로, 이 패치는 깔끔하게 작성되었으며 기본적인 기능은 잘 구현될 것으로 보입니다. 위에서 언급한 사항들에 주목하여 코드의 안정성과 유지 보수성을 더욱 향상시킬 수 있습니다.

import quiz from "./quiz.json";
import tags from "./tags.json";

// eslint-disable-next-line import/no-anonymous-default-export
export default {
[apiRoutes.quiz]: quiz,
[apiRoutes.tags]: tags,
[apiRoutes.problems]: problems,
};

Choose a reason for hiding this comment

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

이 패치 코드를 보면, 기본적으로 문제가 있어 보이지는 않습니다. 그러나 다음과 같은 몇 가지 개선을 고려해 볼 수 있습니다:

  1. 파일 크기: "problems.json", "quiz.json", "tags.json"의 파일들이 매우 큰 경우 브라우저에서 로드하는데 시간이 오래 걸릴 수 있으므로, 데이터 크기에 따라 서버에서 동적으로 로드하도록 구현하는 것이 더 효율적일 수 있습니다.

  2. JSON 파일 검증: JSON 파일의 데이터 형식이 변경될 가능성이 있다면, 프로그램이 이 파일들을 정상적으로 읽을 수 있도록 데이터를 유효성 검사하는 메커니즘을 추가하는 것이 좋습니다.

  3. 주석 처리된 eslint-disable 라인: 이미 이 코드에서 eslint 규칙을 사용하지 않기 위해 주석 처리한 부분이 있는데, 혹시 린팅 규칙을 변경하여 이 부분을 제거할 수는 없는지 고려해보세요. 일반적으로 규칙을 비활성화하는 것보다 코드베이스 전체에서 일관된 코딩 스타일을 유지하는 것이 더 좋습니다.

  4. import 방식: ES6 모듈 구문을 사용하고 있는데, 일부 환경에서는 CommonJS의 require()를 사용해야 할 수도 있습니다. 이 점은 반드시 고려할 사항은 아니지만, 호환성 문제를 미연에 방지하려면 알고 있어야 합니다.

}
]
}
}

Choose a reason for hiding this comment

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

이 코드 패치를 확인해보면, 주어진 JSON 형식의 데이터는 문제(즉, "ETF(상장지수펀드)의 특징이 아닌것은?")와 그에 대한 선택지를 정의합니다. 여기에서 명확한 버그 리스크가 발생하는 부분을 찾기는 어렵습니다.

그러나 개선 방안 약간을 제안하자면 다음과 같습니다:

  1. 필드명 암시: 'creatorId' 필드의 이름은 제작자 자체를 의미하는지, 아니면 제작자 ID를 의미하는지 암시하는 것이 불분명합니다. 필드의 이름을 좀 더 세부적으로 수정하는 것이 명확성 향상에 도움이 될 수 있습니다. 예를 들어, 'questionCreatorId'와 같은 필드명은 더욱 명확하게 이 필드가 어떤 건인지를 전달할 수 있습니다.
  2. 데이터 유효성 검증: 이 패치에 있는 JSON 형식의 데이터에 대해서는 유효성 검사를 할 수 있는 코드가 따로 없습니다. 관련 코드들이 실질적으로 수용하거나 처리하기 전에 데이터의 유효성을 검증하기 위한 체계를 만들면, 잘못된 데이터 입력으로부터 발생할 수 있는 문제들을 막아낼 수 있습니다.

다음에는 실제 코드를 주시면 더욱 구체적인 리뷰를 제공드리겠습니다. 이 내용은 데이터 형식에 대한 설명이므로, 특정 프로그래밍 언어나 실행 환경에 관한 문제점을 파악하거나 코딩 습관 등에 대해 조언드릴 수 있는 점이 제한적입니다.


import { handlers } from "./handlers";

export const server = setupServer(...handlers);

Choose a reason for hiding this comment

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

이 코드 패치는 대체로 잘 작성되었으나 아래의 몇가지 사항들을 고려해 보실 것을 권장합니다:

  1. 의존성 문제: "msw/node" 와 "handlers"가 있어야 실행이 가능하니 두 모듈이 존재하는지 확인하세요. 만약 ./handlers 파일이 존재하지 않는다면, handlers를 임포트 할 수 없으므로 오류가 발생할 수 있습니다.

  2. 타입 확인: JavaScript는 동적 타입 언어이므로, handlers의 내용물이 setupServer 함수에서 예상하는 형식과 일치하는지 확인하는 것이 좋습니다. TypeScript를 사용하거나 주석에 이 정보를 추가하는 것이 훌륭한 방법입니다.

  3. 오류 처리: 오류 처리 로직이 아직 구현되지 않았습니다. 서버 설정 단계에서 문제가 생겼을 때 적절한 오류 메시지를 보여주거나 복구 절차를 진행하는 것이 중요합니다.

마지막으로, 작성하신 패치 및 코드 전반적으로 개발 규칙(스타일 가이드, 네이밍 컨벤션 등)을 따른 것인지 확인해보시는 것도 유용합니다.

@@ -1,4 +1,5 @@
export const apiRoutes = {
quiz: "/quiz",
tags: "/tags",
problems: "/articles/:articleId/problems",
};

Choose a reason for hiding this comment

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

이 코드 조각은 기존의 apiRoutes 객체에 'problems'라는 새로운 property를 추가하고 있습니다. 기능적 문제는 보이지 않습니다; 이 변경 사항은 /articles/:articleId/problems라는 경로를 가리키는 problems 속성을 추가하므로, 원래 의도대로 작동하는 것처럼 보입니다. 이러한 동적 라우트 건설은 상당히 일반적입니다.

그러나 몇 가지 주의점을 고려해보세요:

  1. 문자열 내에 ':articleId'가 동적 경로 변수임을 확인하세요. 이 값이 항상 올바르게 치환되어야 합니다.

  2. 이 변경사항을 활용하는 다른 코드에서 '/articles/:articleId/problems' 경로를 사용할 때 정확하게 ':articleId'를 합당한 ID로 치환하겠다는 보장이 있는지 확인하세요.

  3. 이런 변화가 앱 전반의 라우팅에 어떤 영향을 미칠지 잘 따져 보아야 합니다. 반드시 관련 테스트 케이스를 실행해서 심각한 회귀 문제가 없는지 확인하세요.

  4. apiRoutes의 naming convention과 일관성유지가 중요합니다. 여기서 'problems'라는 이름이 다른 route명과 일관성이 있다면 별도의 문제가 없어 보입니다. 그럼에도 불구하고 작업을 수행하는 개발자는 이 점을 잘 기억해두어야 합니다.

아울러, 주석으로 코드 변경사항의 목적을 명확하게 표시하여 다른 개발자들이 왜 이런 변경사항이 만들어졌는지 이해하는 데 도움이 될 것입니다.

@@ -22,6 +22,8 @@ export default defineConfig({
"@quiz": resolve("src/quiz"),
"@article": resolve("src/article"),
"@main": resolve("src/main"),
"@mocks": resolve("src/mocks"),
"@common": resolve("src/common"),
},
},
});

Choose a reason for hiding this comment

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

이 코드 조각은 전반적으로 깔끔해 보입니다. 일부 추가된 알리아스들("@Mocks", "@common") 이 선언된 특정 영역에 오류가 없는지 액세스 경로를 확인해야 합니다. 찾을 수 없는 디렉토리를 참조하면 문제가 발생할 수 있습니다.

더 나아가기 위해서, 다음과 같은 것들을 고려해볼 수 있습니다:

  1. 어플리케이션의 다른 부분에서 이 새로운 alias를 사용하도록 설계가 되어 있는지.
  2. resolve() 함수가 가장 최신상태인지, 이 때문에 버그가 생길 가능성이 있는 지 확인 필요.

가능한 개선점에 대해서는 이런 점들을 고려해보세요:

  • resolve 함수는 코드 복잡성을 줄여주는데 도움이 될 수 있지만, 어딘가 다른 파일에서 변경될 시 이러한 설정파일 수정이 필요 할 수 있으므로 스스로 무결성을 유지하는 방법을 배치하는게 좋습니다.
  • 여기서 알리아스는 중복 관리를 위한 용도로만 사용되어야합니다. 기능 또는 유효성 검사를 수행하지 않는다는 점을 확인해야 합니다.

이외에도 여러 가지 구체적인 문제가 발생할 수 있으며, 스펙이나 코드 전체 상황에 따라 다르니 이것들을 고려하면서 추가적인 검토가 필요합니다.

vi.clearAllMocks();
});

afterAll(() => {
server.close();
vi.resetAllMocks();
});

Choose a reason for hiding this comment

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

이 패치의 전반적인 목적은 vitest에서 서버 모킹을 관리하기 위함으로 보입니다. 아래에 몇가지 리뷰 사항이 있습니다:

  1. vi.clearAllMocks();: vi는 "vitest"에서 import되었으나, vitest에서 clearAllMocks 함수를 제공하지 않는 것으로 알려져 있습니다. 이 코드는 런타임 에러를 발생시킬 수 있습니다.
  2. server.listen(), server.resetHandlers(), server.close(): 이들은 실제 서버 상태에 영향을 주는 함수로 처리되어야 할 것 같습니다. 서버 객체와 위치(@mocks/server)가 실제로 존재하며, 해당 메서드들이 서버 상태를 적절히 관리하는지 확인해야 합니다.
  3. 테스트 생명주기: beforeAll, afterEach, afterAll 등이 적절한 시점에서 호출되는지 확인하십시오.
  4. 코드정리: 아직 사용되지 않는 import가 있는지 검사하고 없애도록 하세요.

이상입니다. 보다 구체적인 리뷰를 위해서는 컨텍스트가 더 필요합니다.

@Happhee Happhee merged commit 7d2024e into develop Jun 10, 2024
3 checks passed
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.

[ ✨ Init ] msw + vitest 환경 구축 및 next 적용
2 participants