Issue 171: api 공통 로직 분리 및 accessToken 런타임 저장 #183
Conversation
|
Kudos, SonarCloud Quality Gate passed! |
|
이전에 언급되었던 개선 사항을 빠르게 적용해주시느라고 고생하셨습니다! 본격적으로 리뷰를 하려다가, 몇 가지 먼저 궁금한 점이 있어서 질문들 드리고 답변을 들은 다음에 리뷰를 해보려구요!
하나만 더불어 이야기하자면, PR을 만들 때 어떤 선택을 왜 내리게 되었는 지에 대한 설명을 부가해주시면 좋을 것 같아요! 저희가 소통을 계속해서 할 수 있으면 좋겠지만, 아무래도 여러가지 어려움이 있으니 최대한 PR 메시지와 슬랙에 어떤 방향으로 구현을 하려고 했고 어떤 코드는 왜 이런 선택을 했는지 기록을 남겨주시면 코드를 리뷰하기에 훨씬 용이할 것 같아요! |
|
|
||
| if (!accessToken) { | ||
| window.sessionStorage.removeItem(ACCESS_TOKEN); | ||
| if (!ACCESS_TOKEN) { |
There was a problem hiding this comment.
상수가 없을 때가 있나요..? 그럼 이건 변수 아닐까요??
There was a problem hiding this comment.
window.sessionStorage.getItem(ACCESS_TOKEN_KEY)를 하면 넘어오는 타입이 string | null이어서 위처럼 조건문을 처리했습니다. 그런데 if(!ACCESS_TOKEN)보다는 if(ACCESS_TOKEN === null)로 하는 것이 더 명확했을 것 같네요!
src/constants/api.tsx
Outdated
| @@ -40,7 +40,8 @@ export const FILTERS = [ | |||
| { order: "spell", text: "가나다 순" }, | |||
| ] as const; | |||
|
|
|||
| export const ACCESS_TOKEN = "matzipaccessToken"; | |||
| export const ACCESS_TOKEN_KEY = "matzipaccessToken"; | |||
| export const ACCESS_TOKEN = window.sessionStorage.getItem(ACCESS_TOKEN_KEY); | |||
There was a problem hiding this comment.
전역 변수(모듈 변수)로 들고 있으면 리액트 전역 상태로 들고 있을 때와 어떤 차이가 있을까요? 렌더링 관점에서 생각해보면 좋을 것 같습니다 :D
There was a problem hiding this comment.
전역변수는 js 모듈에서 선언된 변수여서 어디서든 쉽게 값을 참조할 수 있지만 값이 변경되어도 React 컴포넌트 변화를 감지하지 못하니까 자동으로 렌더링을 하지 못하는 반면 전역 상태는 변경되면, 해당 상태에 의존하는 모든 컴포넌트는 자동으로 다시 렌더링 되겠네요!
로그아웃 상황에 대해서는 완전 머리속에서 지워버렸었어요....ㅎㅎㅎ 그래서 위처럼 상수로 선언하면 문제가 생길 가능성이 있겠네요...🤔
src/context/LoginContextProvider.tsx
Outdated
| @@ -2,7 +2,7 @@ import React, { useState } from "react"; | |||
|
|
|||
| import { ACCESS_TOKEN } from "constants/api"; | |||
|
|
|||
| const hasAccessToken = !!window.sessionStorage.getItem(ACCESS_TOKEN); | |||
| const hasAccessToken = !!ACCESS_TOKEN; | |||
src/api/utils/index.ts
Outdated
| @@ -18,3 +18,9 @@ export const checkAndSetToken = (config: AxiosRequestConfig) => { | |||
|
|
|||
| return config; | |||
| }; | |||
|
|
|||
| export const handleAPIError = (error: AxiosError) => { | |||
| const { data } = error.response as AxiosResponse; | |||
There was a problem hiding this comment.
as 말고 if문으로 분기처리하는 건 어떨까요?
| import { API_BASE_URL } from "constants/api"; | ||
| import { ACCESS_TOKEN, API_BASE_URL } from "constants/api"; | ||
|
|
||
| import { checkAndSetToken, handleAPIError } from "api/utils"; |
There was a problem hiding this comment.
음... 뭔가 axiosInstance에서 파일에서 interceptor 내부 로직(?)을 분리하고 싶은데, 특별히 네이밍을 뭘 하면 좋을지 떠오르지 않아서 util이라고 했습니다.
src/api/axiosInstance.tsx
Outdated
|
|
||
| throw Error(data.message); | ||
| }; | ||
| export const authAxiosInstance = axios.create({ |
There was a problem hiding this comment.
authAxiosInstance와 axiosInstance를 분리한 이유가 무엇인가요? api에 따른 토큰 처리를 프론트에서 하는 게 맞나요??
저도 잘 모릅니다ㅎㅎ 근데 토큰 자체만 보면 프론트에서는 사실상 의미 없는 텍스트고 서버에서 처리할 때만 의미가 생기니까 프론트에서는 있으면 보내주고 없으면 안보내주고 이런 식으로 해야하지 않을까 하는 생각입니다!
There was a problem hiding this comment.
가장 먼저 두 개를 나눈 이유는 프론트에서도 이 요청이 accessToken을 필요로 하는지 안 하는지를 알면 좋지 않을까라는 생각이 들어서 그렇게 했습니다! 그랬던 이유가, 음식점 목록 요청 같은 경우에는 access token을 필요로 하지 않는데, 현재 accessToken을 소유하고 있다고 토큰을 필요로 하지 않는 api에 보내줘도 되는 건지를 잘 모르겠더라고요. 사용할 때 이건 accessToken이 필요 하구나 아니면 없어도 되는 것이라는 것을 알고 있으면 도움이 되지 않을까 했어요. 거기에 에러처리 로직도 interceptor에 넣은 다음에 accessToken의 유무에 따라서 처리를 하면 무조건 로그인 상태가 아닐 때는 에러가 발생하지 않을까라는 생각도 들었습니다. 그래서 무조건 accessToken이 있다고 보내주는 대신 일단 인스턴스를 2개로 분리했습니다!
그런데 custom config를 추가할 수 있다는 글을 보고, 인스턴스 하나로 token 필요 여부 플래그를 만들어서 처리하는 방식으로 리팩토링 해봤습니다.
이것 외에는 아직 axios가 익숙치 않아서 잘 모르겠습니다! 보통은 이런 상황에서 어떻게 처리하나요?
전처럼 axios 인스턴스를 두 개로 나누는 대신 useAuth라는 custom config를 사용해서 header에 authorization 정보를 포함해야 하는지를 검증
1.로그아웃에 대해서는 완전히 생각 못하고 있었네요...! 상수로 선언하면 로그아웃을 했을 때 sessionStorage에서는 토큰이 삭제되지만, 앱을 사용하는 동안에는 계속 남아있게 됩니다. 마찬가지로 로그아웃 상태에서 다시 로그인을 하면 토큰이 계속 null인 상수이기 때문에 문제가 생기겠네요. 그런데 그렇다고 let으로 선언해서 사용하는 것도 좋은 방법인지는 잘 모르겠습니다. 그래서 차라히 매번 sessionStorage에 접근해서 accessToken을 가져오는 것이 더 안전할 것 같아서 수정했습니다. 2.처음에 가장 먼저 두 개를 나눈 이유는 프론트에서도 이 요청이 accessToken을 필요로 하는지 안 하는지를 알면 좋지 않을까라는 생각이 들어서 그렇게 했습니다! 그랬던 이유가, 음식점 목록 요청 같은 경우에는 access token을 필요로 하지 않는데, 현재 accessToken을 소유하고 있다고 토큰을 필요로 하지 않는 api에 보내줘도 되는 건지를 잘 모르겠더라고요. 사용할 때 이건 accessToken이 필요 하구나 아니면 없어도 되는 것이라는 것을 알고 있으면 도움이 되지 않을까 했어요. 거기에 에러처리 로직도 interceptor에 넣은 다음에 accessToken의 유무에 따라서 처리를 하면 무조건 로그인 상태가 아닐 때는 에러가 발생하지 않을까라는 생각도 들었습니다. 현재 로그인 상태에서는 요청할 때마다 accessToken을 보내줘야 하지만, 로그아웃 했을 때 에러 처리를 accessToken의 유무로 한다면 이 처리를 어떻게 해야할지 잘 감이 오지 않았습니다. 그래서 무조건 accessToken이 있다고 보내주는 대신 일단 인스턴스를 2개로 분리했습니다! 그런데 custom config를 추가할 수 있다는 글을 보고, 인스턴스 하나로 token 필요 여부 플래그를 만들어서 처리하는 방식으로 리팩토링 해봤습니다. 인스턴스를 하나만 사용하고 인스턴스 사용처에서 Authorization 정보가 필요한 요청인지 아닌지를 알려준 다음에 interceptor에서 이 정보에 따라 다르게 처리를 하고 있습니다. 이것 외에는 아직 axios가 익숙치 않아서 잘 모르겠습니다! 보통은 이런 상황에서 어떻게 처리하나요? |
Issue #171
accessToken을 런타임에서 들고 있는다Authorization: Bearer ${accessToken}을 api 요청 보내기 전에 미리 처리한다