-
Notifications
You must be signed in to change notification settings - Fork 0
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
[코드 리뷰] 8주차 코드리뷰 #118
[코드 리뷰] 8주차 코드리뷰 #118
Conversation
전체 파일 구조 리팩토링
환경변수 경로 변경
전체적인 Router 수정
export const fetchInstance = initInstance({ | ||
baseURL: BASE_URI, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BASE_URI 를 굳이 환경변수로 관리할 필요가 있을까요..? 그것도 감춰서...?
어차피 배포하면 다 클라이언트에 노출될꺼라서요.
오히려 제가 리뷰할 때 어플리케이션을 실행할 수도 없고..
지금처럼 만들 이유 자체가 없어보여요 ㅠㅠ
제가 BFF 언급한 이유는
서버에 값을 감출 수 있기 때문인데
지금은 값을 감추는 용도로 쓰이는 것도 아니고
그냥 환경에 따라 핸들링을 하는건데
여튼 결론은 "굳이 환경변수를 감춰야돼?" 입니다.
그럴 필요가 지금은 전혀 없어보여요.
} from 'axios'; | ||
import axios from 'axios'; | ||
|
||
import { authLocalStorage } from '@/shared'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shared에서 shared를 alias를 통해 접근하게 되면 순환참조가 발생할 수 있답니다..!
상대경로를 통해 직접 접근을 해주세요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
전혀 생각을 못했네요.. 수정하겠습니다!
src/shared/api/instance/Instance.tsx
Outdated
import axios from 'axios'; | ||
|
||
import { authLocalStorage } from '@/shared'; | ||
import { BASE_URI } from '@/shared/utils/env/config'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
앞선 내용과 동일합니다.
const response = await fetchInstance.post(`${pointApiPath}/withdraw`, { | ||
price: price, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const response = await fetchInstance.post(`${pointApiPath}/withdraw`, {
price,
});
이렇게 생략해서 표현할 수 있어요!
return { get, set }; | ||
}; | ||
|
||
export const authLocalStorage = initStorage('accessToken', localStorage); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
authStorage
처럼 표현하면 어떨까요? local 을 붙일 필요는 없어보여요~
const handleBackClick = () => { | ||
if (window.history.length > 1) { | ||
navigate(-1); | ||
} else { | ||
navigate(defaultBackPath); | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
goToBack
처럼 표현해야 더 자연스러울 것 같아요.
이벤트 핸들러는 역할으 제한적이지만
goToBack
처럼 표현하면 더 범용적으로 사용할 수 있답니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
그렇군요! 참고해서 수정하겠습니다!
@@ -1,7 +1,7 @@ | |||
import { Link } from 'react-router-dom'; | |||
|
|||
import Logo from '../../assets/kakao.svg'; | |||
import { KAKAO_AUTH_URL } from '@/shared/constants'; | |||
import { KAKAO_AUTH_URL } from '@/shared/utils/env/config'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
마찬가지로 이걸 왜 환경변수에서 가져와야 하는지 잘 모르겠어요 ㅎㅎ;
const [isMore, setIsMore] = useState(false); | ||
const [isEditing, setIsEditing] = useState(false); | ||
const [guidelineTitle, setGuidelineTitle] = useState(guideline.title); | ||
const [guidelineContent, setGuidelineContent] = useState(guideline.content); | ||
|
||
const editMutation = useModifyGuideline(refetch, guideline.id); | ||
const deleteMutation = useDeleteGuideline(refetch, guideline.id); | ||
|
||
const toggleContent = () => { | ||
setIsMore(!isMore); | ||
}; | ||
|
||
const handleEdit = () => { | ||
editMutation.mutate({ | ||
seniorId: seniorId, | ||
type: guideline.type, | ||
title: guidelineTitle, | ||
content: guidelineContent, | ||
}); | ||
setIsEditing(false); | ||
}; | ||
|
||
const handleDelete = () => { | ||
deleteMutation.mutate(guideline.id); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
지금 custom hook으로 만들어서 사용하는게 전부 api 관련 로직들인데
이런 컴포넌트 내부의 로직도 custom hook 으로 만들어서 관리할 수 있답니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
무슨 말인지 이해했습니다! custom hook 으로 빠질 수 있는 api 관련 내용들은 분리하도록 하겠습니다!
const handleDelete = () => { | ||
deleteMutation.mutate(guideline.id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleDelete => deleteGuideLine
const handleEdit = () => { | ||
editMutation.mutate({ | ||
seniorId: seniorId, | ||
type: guideline.type, | ||
title: guidelineTitle, | ||
content: guidelineContent, | ||
}); | ||
setIsEditing(false); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handleEdit => edit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
변경사항이 너무 커서 file changes 에서 파일을 조회하는게... 거의 불가능하네요 ㅎㅎ;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
백엔드 분들께 '배포한 FE 사이트도 카카오 로그인 할 수 있게 등록해주세요!' 라고 의견을 전달했더니, Redirect Page를 현재 localhost:5173/redirection 으로 등록을 해놔서 다른 주소로 바꿀 수가 없다고 합니다. 처음에는 그냥 2개 (로컬이랑 배포 환경) 다 등록하면 되지 않나? 했는데 생각해보니까 리다이렉트 페이지를 2개 등록할 수 있나? 라는 의문점이 생겼습니다.
하지만 옛날에 다른 팀원들이랑 협업을 하며 카카오 로그인을 진행해 본 기억이 있어 주변 백엔드 분들에게 물어보니 벡엔드에서 조건문(?) 처리를 통해 리다이렉트 페이지를 여러개 등록할 수 있다는 의견과 개발 서버와 배포 서버 두개를 운영해서 각각 로그인이 가능하게 하는 방법이 있다는 의견으로 총 2가지 의견을 받아볼 수 있었습니다.
아마 kakao develop 사이트에서의 redirect page 관련한 이슈 같은데,,, 일단 이 부분은 시험이 끝난 후에 백엔드분들이랑 의논해보기로 했습니다! 따라서 현재는 local 환경에서만 카카오 로그인이 가능합니다... ㅠㅠ 혹시라도 더 좋은 방법이 있을까요...?
redirection 을 하는 목적 자체를 생각해보면 좋을 것 같아요.
카카오에 로그인을 시도하면 토큰을 가져온 다음에 redirection 페이지에게 넘겨주는 구조입니다.
그래서 redirection이 저는 꼭 프론트에 있어야하나? 라는 생각이 있어요
백엔드에서 관리하고 백엔드에서 처리하도록 하는거죠 ㅎㅎ
공식문서를 보면 알겠지만 결국 redirect는 서버로 해야돼요.
localhost는 개발용 서버인거고 이걸 실제로 서버로 쓰진 않으니까요.
이건 서버 개발자들이 더 고민하고 해결해야 하는 부분인 것 같아요.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이유는 모르겠지만, RefreshToken을 이용해 AccessToken을 발급 받는 기능이 잘 동작하지 않는 것 같습니다.. 이 부분도 accessToken의 유효기간(?) 을 일시적으로 줄여서 토큰 재발급이 정상적으로 될 수 있도록 한번 수정해보겠습니다..! 시험이 끝나면...
이건 어떤 코드에 해당 기능이 있는지 알려주셔야 할 것 같아요.. ㅎㅎ
환경변수 관련 문제 때문에 지금 제가 로컬에서 실행이 불가능합니다.
localStorage.removeItem('refreshToken'); | ||
} | ||
return Promise.reject(error); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
여기가 토큰 갱신 로직이군요..
이건 실행되는 모습을 봐야 뭐가 문제인지 알 수 있을 것 같네요 ㅠㅠ
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
혹시 방금 서버 주소를 추가했는데 확인해주실수 있을까요...? ㅠㅠ
!Hotfix: 서버 주소 추가
https://developers.kakao.com/docs/latest/ko/kakaologin/prerequisite#kakao-login-redirect-uri 와.. 이 부분은 저도 못보고 지나간것 같네요..! 이 글을 바탕으로 한번 의논해보겠습니다. 감사합니다! |
일단 이 PR은 머지하겠습니다! |
📝 작업 내용
💬 리뷰 요구사항(선택)
안녕하세요 멘토님! 이번주차부터 다음주차까지는 저희 학교 시험기간이라 참 바빴던 한 주 였던것 같습니다 ㅜㅜ
먼저 저번 코드리뷰와 멘토링시간에 멘토님이 알려주신 부분들을 일부 반영은 했으나, 완벽하게 반영안된부분들도 존재합니다! 프론트는 3명 다 시험을 안쳐서 계속 개발을 진행하고 있지만, 백엔드에서 시험을 치는 팀원들이 일부 있어 아마 시험이 끝난 후에 해당 담당자랑 소통하면서 나머지 부분들도 수정해볼려고 합니다.
먼저, 저번 리뷰에서 '백엔드 주소'관련 이슈가 있었는데, 이것저것 찾아보니까 submodule이라는 기술이 있길래 한번 도입해보았습니다! 멘토님이 제안해주신 BFF도 고려해보았는데, 몇번 읽어도 무슨 말인지 잘 이해가 안가서.. 열심히 공부해서 다음기회에는 꼭 도입해보겠습니다...!
Subboudle
Git Submodule을 사용해보자 이 글을 참고해서 적용해보았습니다!
멘토님이 로컬환경에서 clone하고 사용하실때는
해당 명령어를 통해 환경변수를 가져올 수 있습니다!
로그인
백엔드 분들께 '배포한 FE 사이트도 카카오 로그인 할 수 있게 등록해주세요!' 라고 의견을 전달했더니, Redirect Page를 현재
localhost:5173/redirection
으로 등록을 해놔서 다른 주소로 바꿀 수가 없다고 합니다. 처음에는 그냥 2개 (로컬이랑 배포 환경) 다 등록하면 되지 않나? 했는데 생각해보니까 리다이렉트 페이지를 2개 등록할 수 있나? 라는 의문점이 생겼습니다.하지만 옛날에 다른 팀원들이랑 협업을 하며 카카오 로그인을 진행해 본 기억이 있어 주변 백엔드 분들에게 물어보니 벡엔드에서 조건문(?) 처리를 통해 리다이렉트 페이지를 여러개 등록할 수 있다는 의견과 개발 서버와 배포 서버 두개를 운영해서 각각 로그인이 가능하게 하는 방법이 있다는 의견으로 총 2가지 의견을 받아볼 수 있었습니다.
아마 kakao develop 사이트에서의 redirect page 관련한 이슈 같은데,,, 일단 이 부분은 시험이 끝난 후에 백엔드분들이랑 의논해보기로 했습니다! 따라서 현재는 local 환경에서만 카카오 로그인이 가능합니다... ㅠㅠ
혹시라도 더 좋은 방법이 있을까요...?
dist파일 eslintignore 처리
vite환경에서 build시 나타나는 dist 폴더를 eslintignore에 등록해두어 해당 부분은 수정되었습니다!
⏰ 현재 버그
이유는 모르겠지만, RefreshToken을 이용해 AccessToken을 발급 받는 기능이 잘 동작하지 않는 것 같습니다.. 이 부분도 accessToken의 유효기간(?) 을 일시적으로 줄여서 토큰 재발급이 정상적으로 될 수 있도록 한번 수정해보겠습니다..! 시험이 끝나면...
멘토링 시간에 전달해주신 리액트 관련 자료 너무 잘 쓰겠습니다 멘토님 ㅠㅠ 참고해서 꼭 리액트 직접구현을 성공할 수 있도록 해보겠습니다! 열심히 구르겠습니다.. 데굴데굴..
아 그리고 LCRS 트리부터 시작하는게 맞을까요..? 제가 비전공자다 보니 알고리즘이나 cs 관련된 부분은 잘 모르는데.. 스터디하는 팀원들이 계속 괴롭혀서.. ㅠㅠ