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_김지안_6주차 과제 #95

Open
wants to merge 26 commits into
base: kimji-an
Choose a base branch
from

Conversation

KimJi-An
Copy link

@KimJi-An KimJi-An commented Aug 3, 2024

안녕하세요, 멘토님! 충남대학교 김지안 입니다.

단계 별 리뷰를 받고자 1단계를 먼저 제출하였으나, 개인 사정으로 일요일에 컴퓨터를 거의 사용하지 못할 것 같아 한 번에 다시 제출합니다. 여기에서 한 번에 리뷰 해주셔도 좋을 것 같아요.

API 연결은 팀원 3분과 진행해보았습니다.
로컬에서 회원 가입, 로그인, 카테고리 목록 조회, 상품 목록 조회, 상품 조회까지는 동작이 확인되었으나, 위시 리스트 부분은 CORS 에러로 인해 백엔드 측에서 계속 수정 진행 중이라 아직 확인하지 못했습니다. 그리고 프론트엔드와 백엔드의 프로토콜이 일치하지 않아 에러가 발생하고 있으나, 로컬에서 확인하고 되는 데까지 제출하라고 하셔서 일단 제출합니다.

마지막 주차라 계획대로 진행해서 리뷰를 받고 싶었는데, 많은 팀원들과 맞춰 보는 건 처음이라 일정이 계속 미뤄져 많이 아쉽습니다.

6주간 좋은 리뷰 주셔서 정말 감사드립니다! 마지막 리뷰도 잘 부탁드립니다.

Copy link

@taehwanno taehwanno left a comment

Choose a reason for hiding this comment

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

피드백 드려요!

const authHeader = req.headers.get('Authorization');
if (!authHeader) {
return res(ctx.status(401));
}

Choose a reason for hiding this comment

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

msw에서 higher-order resolver를 활용한다면 각 API mocking handler에서 반복될 수 있는 인증 처리를 간소화할 수 있어요. 문서에 있는 것처럼 withAuth를 만들어서 적용하면 5~8번 라인은 제거할 수 있겠죠. 참고 부탁드려요.

src/api/hooks/wishlist.mock.ts Outdated Show resolved Hide resolved
): Promise<WishlistResponseData> => {
const url = getWishlistPath(params, apiUrl);

const response = await fetch(url, {

Choose a reason for hiding this comment

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

다른 API 호출 함수는 axios를 사용하는데 여기에서는 fetch를 사용하고 있네요? http client는 일관되도록 맞춰주는게 좋습니다. axios를 통해 호출하는 코드는 각종 axios config와 interceptor가 적용되겠지만 fetch는 그러지 못할텐데 일관되지 않은 동작은 제품 개발과 디버깅에 어려움을 주는 요소일 거라서요.

fetch로 구현한 getWishlist는 apiUrl을 일일이 넘겨줘야한다는 단점도 있죠.

const apiUrl = localStorage.getItem('apiUrl') || backend.backend3;
config.baseURL = apiUrl;

return config;

Choose a reason for hiding this comment

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

axios instance의 요청할 때 마다 apiUrl을 주는게 아니라 Header에서 API 타겟 주소가 변경될 때 instance의 기본 baseURL을 변경해주면 15~25번 라인은 제거 가능해보입니다.

Header에서 useApi의 setApiUrl 에서 변경되도록 구현을 개선해볼 수 있겠네요.

instance.defaults.baseURL = /* ... */


### 🌿 2단계 - 배포하기
- github pages를 사용하여 배포

Choose a reason for hiding this comment

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

배포하신 github pages 링크 첨부해주시겠어요? github pages에서 SPA 지원은 기본적으로 하지 않기에 의도적으로 몇 가지 설정을 해줘야할텐데 그게 코드로 보이진 않은데 한번 내용 공유해주시면 좋겠습니다.

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