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

refactor: 맵 리스트를 API 명세에 맞게 마이그레이션 #976

Merged
merged 6 commits into from
Nov 2, 2023

Conversation

2yunseong
Copy link
Collaborator

@2yunseong 2yunseong commented Oct 24, 2023

구현 기능

API 분리

일단은 기존의 찜꽁의 기능과 현재 개발중인 기능을 mocking 함수에서 사용하기 위해, 기존의 api/api.ts 파일을 그대로 복사해
api-v2/api.ts 를 생성했습니다. 일단은 BASE_URL을 하드코딩없이 localhost 주소를 고정해두었습니다.

mockoon으로 mocking 할 경우, 7742번 포트를 사용해주시기 바랍니다. 시간 상 임의로 정한 점 양해 부탁드립니다!

API 명세에 따른 기능 구현

명세는 Open API 문서 를 참고해주시면 됩니다. 여기서 구현된 기능은 이슈(#975)에서 말씀드렸듯이, Map에 관한 GET, DELETE 기능이 마이그레이션이 필요했습니다.

  • GET /api/maps 관련해서, api 명세상 notice와, sharingMapId 가 사라져, 해당 버튼을 컴포넌트에서 삭제했습니다.
  • GET /api/maps 관련해서, 기존의 데이터 스키마를 바꾸는 명세가 코드 상으론 약간..은 부담스러워 백엔드에게 요청했습니다.
    • 주노 감사합니다 ㅎㅎ (아마 수정된 사항은 바로 API 명세 문서에 반영될 것 같아요. 하지만 참고하셔야 될 것 같아 알려드립니다.)
  • 삭제 버튼도 지금의 API 명세에 맞게 변경했습니다.
  • User 정보를 불러오는 부분을 삭제했습니다.

아래 변경 전/후 스크린샷을 보면 어떤 점이 변경되었는지 가시적으로 이해하실 수 있으리라 생각합니다. 참고바랍니다.

변경 전 변경 후
image image

논의하고 싶은 내용

또 api v2를 하면서 고민한 부분이 있습니다. 일단은 api 관련 로직과 타입은 새로 정의해주었는데, (v2 postfix 로 작성된 네이밍은 모두 새로정의된 부분입니다.) query등은 기존의 코드를 수정하였습니다. query는 굳이 새로 정의해줄 필요가 없고 기존의 코드를 변경하면 좋다고 생각했는데, 어떻게 생각하시나요?

  • 개발 중 query 같은 경우도 함께 V2로 이전해야겠다고 느꼈습니다. 그 이유는 refactor: 맵 편집/생성 페이지를 API 명세에 맞게 마이그레이션 #979 에서 맵 단건 조회 API 를 변경하는데, 이 맵 단건 조회 API가 다른 페이지에서도 사용되다보니 만약 기존의 코드를 변경한다면 다른 페이지에도 영향을 끼쳐 작업 범위를 넘어가게 해 복잡해질 것 같아 query자체도 분리하면 좋을 것 같습니다!

Close #975

Copy link
Collaborator

@suyoungj suyoungj left a comment

Choose a reason for hiding this comment

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

마이그레이션 사항 확인했습니다!!

리뷰 이전에 궁금한 점이 있는데,

  1. fork한 레포지토리에서 작업하고 merge하는 방식으로 계속 하시는걸까요?
  2. PR 타이틀을 이전 PR들과 일치시키는게 좋아 보여요!

나머지는 코멘트로 남겼습니다. 확인 부탁드려요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

query 래핑 커스텀 훅도 V2로 이전하는 것이 좋아보이네요! 제 작업 브랜치에도 반영하도록 하겠습니다 👍

[QueryKey]
>
): UseQueryResult<TData, AxiosError<ErrorResponse>> =>
useQuery(['getManagerMapsV2'], queryManagerMapsV2, { ...options });
Copy link
Collaborator

Choose a reason for hiding this comment

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

options 대신 {...options}를 사용하신 이유가 궁금합니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

현재 기존에도 발생하는 에러인데 options를 사용하면 다음 타입 에러가 발생합니다.

Argument of type 'UseQueryOptions<AxiosResponse<QueryManagerMapsSuccess>, AxiosError<ErrorResponse>, TData, [QueryKey]> | undefined' is not assignable to parameter of type 'UseQueryOptions<AxiosResponse<QueryManagerMapsSuccess>, AxiosError<ErrorResponse>, TData, QueryKey> | undefined'.
  Type 'UseQueryOptions<AxiosResponse<QueryManagerMapsSuccess>, AxiosError<ErrorResponse>, TData, [QueryKey]>' is not assignable to type 'UseQueryOptions<AxiosResponse<QueryManagerMapsSuccess>, AxiosError<ErrorResponse>, TData, QueryKey>'.
    Types of property 'queryFn' are incompatible.
      Type 'QueryFunction<AxiosResponse<QueryManagerMapsSuccess>, [QueryKey]> | undefined' is not assignable to type 'QueryFunction<AxiosResponse<QueryManagerMapsSuccess>, QueryKey> | undefined'.
        Type 'QueryFunction<AxiosResponse<QueryManagerMapsSuccess>, [QueryKey]>' is not assignable to type 'QueryFunction<AxiosResponse<QueryManagesSuccess>, QueryKey>'.
          Type 'QueryKey' is not assignable to type '[QueryKey]'.
            Type 'string' is not assignable to type '[QueryKey]'.ts(2345)

오류를 해결해주기 위해 Type선언 부분을 바꿀까 하다가 일단은 정확히 알지 못해 기술부채로 남겨두고 rest 연산자를 통해 해결하는 방향으로 개선했습니다!

@2yunseong
Copy link
Collaborator Author

@suyoungj

  1. 익숙한 방식으로 하다보니, 해당 방식으로 계속 진행했습니다! 혹시 걸리시는 부분이 있으신가요?
  2. 수정하겠습니다! 감사합니다 :)

@2yunseong 2yunseong changed the title [FE] 맵 리스트를 API 명세에 맞게 마이그레이션한다. refactor: 맵 리스트를 API 명세에 맞게 마이그레이션한다. Oct 26, 2023
@2yunseong 2yunseong changed the title refactor: 맵 리스트를 API 명세에 맞게 마이그레이션한다. refactor: 맵 리스트를 API 명세에 맞게 마이그레이션 Oct 26, 2023
@2yunseong 2yunseong merged commit a99cde1 into woowacourse-teams:campus-dev Nov 2, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants