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

[코드 리뷰] 8주차 코드리뷰 #118

Merged
merged 255 commits into from
Oct 28, 2024
Merged

[코드 리뷰] 8주차 코드리뷰 #118

merged 255 commits into from
Oct 28, 2024

Conversation

Dobbymin
Copy link
Contributor

@Dobbymin Dobbymin commented Oct 25, 2024

📝 작업 내용

이번 PR에서 작업한 내용을 간략히 설명해주세요.(이미지 첨부 가능)

💬 리뷰 요구사항(선택)

안녕하세요 멘토님! 이번주차부터 다음주차까지는 저희 학교 시험기간이라 참 바빴던 한 주 였던것 같습니다 ㅜㅜ
먼저 저번 코드리뷰와 멘토링시간에 멘토님이 알려주신 부분들을 일부 반영은 했으나, 완벽하게 반영안된부분들도 존재합니다! 프론트는 3명 다 시험을 안쳐서 계속 개발을 진행하고 있지만, 백엔드에서 시험을 치는 팀원들이 일부 있어 아마 시험이 끝난 후에 해당 담당자랑 소통하면서 나머지 부분들도 수정해볼려고 합니다.

먼저, 저번 리뷰에서 '백엔드 주소'관련 이슈가 있었는데, 이것저것 찾아보니까 submodule이라는 기술이 있길래 한번 도입해보았습니다! 멘토님이 제안해주신 BFF도 고려해보았는데, 몇번 읽어도 무슨 말인지 잘 이해가 안가서.. 열심히 공부해서 다음기회에는 꼭 도입해보겠습니다...!

Subboudle

Git Submodule을 사용해보자 이 글을 참고해서 적용해보았습니다!
멘토님이 로컬환경에서 clone하고 사용하실때는

git clone https://github.com/kakao-tech-campus-2nd-step3/Team8_FE.git

git submodule init

git submodule update

해당 명령어를 통해 환경변수를 가져올 수 있습니다!

로그인

백엔드 분들께 '배포한 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 관련된 부분은 잘 모르는데.. 스터디하는 팀원들이 계속 괴롭혀서.. ㅠㅠ

Dobbymin and others added 30 commits October 13, 2024 00:19
전체 파일 구조 리팩토링
Comment on lines +28 to +30
export const fetchInstance = initInstance({
baseURL: BASE_URI,
});

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';

Choose a reason for hiding this comment

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

shared에서 shared를 alias를 통해 접근하게 되면 순환참조가 발생할 수 있답니다..!
상대경로를 통해 직접 접근을 해주세요.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

전혀 생각을 못했네요.. 수정하겠습니다!

import axios from 'axios';

import { authLocalStorage } from '@/shared';
import { BASE_URI } from '@/shared/utils/env/config';

Choose a reason for hiding this comment

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

앞선 내용과 동일합니다.

Comment on lines +26 to +28
const response = await fetchInstance.post(`${pointApiPath}/withdraw`, {
price: price,
});

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);

Choose a reason for hiding this comment

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

authStorage 처럼 표현하면 어떨까요? local 을 붙일 필요는 없어보여요~

Comment on lines +21 to +27
const handleBackClick = () => {
if (window.history.length > 1) {
navigate(-1);
} else {
navigate(defaultBackPath);
}
};

Choose a reason for hiding this comment

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

goToBack 처럼 표현해야 더 자연스러울 것 같아요.

이벤트 핸들러는 역할으 제한적이지만
goToBack 처럼 표현하면 더 범용적으로 사용할 수 있답니다.

Copy link
Collaborator

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';

Choose a reason for hiding this comment

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

마찬가지로 이걸 왜 환경변수에서 가져와야 하는지 잘 모르겠어요 ㅎㅎ;

Comment on lines +22 to +46
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);
};

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 으로 만들어서 관리할 수 있답니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

무슨 말인지 이해했습니다! custom hook 으로 빠질 수 있는 api 관련 내용들은 분리하도록 하겠습니다!

Comment on lines +44 to +45
const handleDelete = () => {
deleteMutation.mutate(guideline.id);

Choose a reason for hiding this comment

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

handleDelete => deleteGuideLine

Comment on lines +34 to +42
const handleEdit = () => {
editMutation.mutate({
seniorId: seniorId,
type: guideline.type,
title: guidelineTitle,
content: guidelineContent,
});
setIsEditing(false);
};

Choose a reason for hiding this comment

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

handleEdit => edit

Copy link

@JunilHwang JunilHwang left a comment

Choose a reason for hiding this comment

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

변경사항이 너무 커서 file changes 에서 파일을 조회하는게... 거의 불가능하네요 ㅎㅎ;

Copy link

@JunilHwang JunilHwang left a 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는 개발용 서버인거고 이걸 실제로 서버로 쓰진 않으니까요.

이건 서버 개발자들이 더 고민하고 해결해야 하는 부분인 것 같아요.

Copy link

@JunilHwang JunilHwang left a comment

Choose a reason for hiding this comment

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

먼저, 저번 리뷰에서 '백엔드 주소'관련 이슈가 있었는데, 이것저것 찾아보니까 submodule이라는 기술이 있길래 한번 도입해보았습니다! 멘토님이 제안해주신 BFF도 고려해보았는데, 몇번 읽어도 무슨 말인지 잘 이해가 안가서.. 열심히 공부해서 다음기회에는 꼭 도입해보겠습니다...!

이걸 환경변수로 관리하는 이유 자체를 모르겠어요.
어차피 배포하면 백엔드 주소는 무조건 노출되는데
코드상에서 감춘다고 어떤 효과가 있는걸까요?

image

Copy link

@JunilHwang JunilHwang left a comment

Choose a reason for hiding this comment

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

이유는 모르겠지만, RefreshToken을 이용해 AccessToken을 발급 받는 기능이 잘 동작하지 않는 것 같습니다.. 이 부분도 accessToken의 유효기간(?) 을 일시적으로 줄여서 토큰 재발급이 정상적으로 될 수 있도록 한번 수정해보겠습니다..! 시험이 끝나면...

이건 어떤 코드에 해당 기능이 있는지 알려주셔야 할 것 같아요.. ㅎㅎ

환경변수 관련 문제 때문에 지금 제가 로컬에서 실행이 불가능합니다.

@Dobbymin
Copy link
Contributor Author

백엔드 분들께 '배포한 FE 사이트도 카카오 로그인 할 수 있게 등록해주세요!' 라고 의견을 전달했더니, Redirect Page를 현재 localhost:5173/redirection 으로 등록을 해놔서 다른 주소로 바꿀 수가 없다고 합니다. 처음에는 그냥 2개 (로컬이랑 배포 환경) 다 등록하면 되지 않나? 했는데 생각해보니까 리다이렉트 페이지를 2개 등록할 수 있나? 라는 의문점이 생겼습니다.
하지만 옛날에 다른 팀원들이랑 협업을 하며 카카오 로그인을 진행해 본 기억이 있어 주변 백엔드 분들에게 물어보니 벡엔드에서 조건문(?) 처리를 통해 리다이렉트 페이지를 여러개 등록할 수 있다는 의견과 개발 서버와 배포 서버 두개를 운영해서 각각 로그인이 가능하게 하는 방법이 있다는 의견으로 총 2가지 의견을 받아볼 수 있었습니다.
아마 kakao develop 사이트에서의 redirect page 관련한 이슈 같은데,,, 일단 이 부분은 시험이 끝난 후에 백엔드분들이랑 의논해보기로 했습니다! 따라서 현재는 local 환경에서만 카카오 로그인이 가능합니다... ㅠㅠ 혹시라도 더 좋은 방법이 있을까요...?

redirection 을 하는 목적 자체를 생각해보면 좋을 것 같아요. 카카오에 로그인을 시도하면 토큰을 가져온 다음에 redirection 페이지에게 넘겨주는 구조입니다. 그래서 redirection이 저는 꼭 프론트에 있어야하나? 라는 생각이 있어요 백엔드에서 관리하고 백엔드에서 처리하도록 하는거죠 ㅎㅎ

공식문서를 보면 알겠지만 결국 redirect는 서버로 해야돼요.

localhost는 개발용 서버인거고 이걸 실제로 서버로 쓰진 않으니까요.

이건 서버 개발자들이 더 고민하고 해결해야 하는 부분인 것 같아요.

사실 첫 멘토링 시간에도 멘토님이랑 의논하고 난 후 백엔드 분들이랑 얘기를 나눠봤는데, 저는 아무리 생각해도 멘토님 말씀처럼 서버에서 처리하는 방법이 맞다고 생각하는데 백엔드분들 입장은 지금 저희 프로젝트 페이지가 로그인을 하게되면 1. 시니또, 2. 보호자 로 구별을 해서 각각의 ui로 이동을 해야해서 시니또인지 보호자인지 회원가입을 통에 구분을 한 후, 로그인 할때마다 관련 정보를 넘겨주어야 해서 프론트에서 처리하는게 맞다고 해서 이 방법을 선택했습니다..
image

인가코드를 서버로 보내주면 아래와 같은 값들이 들어옵니다..
image

사실 저도 이 정보들이랑 redirect page는 별개의 문제라고는 생각하는데.. 백엔드 쪽에서도 멘토링을 통해서 "문제없이 잘 설계한것 같다." 라고 계속 말해서 일단 프론트에 redirect를 해놓고 로그인을 진행하고 있습니다 ㅠㅠ

계속해서 설명을 했는데도 제가 설명을 잘 못하는건지 설득이 안되더라구요..

localStorage.removeItem('refreshToken');
}
return Promise.reject(error);
}

Choose a reason for hiding this comment

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

여기가 토큰 갱신 로직이군요..
이건 실행되는 모습을 봐야 뭐가 문제인지 알 수 있을 것 같네요 ㅠㅠ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

혹시 방금 서버 주소를 추가했는데 확인해주실수 있을까요...? ㅠㅠ

@JunilHwang
Copy link

회원가입을 통에 구분을 한 후, 로그인 할때마다 관련 정보를 넘겨주어야 해서 프론트에서 처리하는게 맞다고 해서

앗.. 그렇군요 ㅠㅠ

말씀해주신 방식에서는 클라이언트에서 해야하는게 맞는 것 같아요.

image

url 10개까지 등록 가능하기 때문에 환경별로 등록해주면 어떨까요?

@Dobbymin
Copy link
Contributor Author

회원가입을 통에 구분을 한 후, 로그인 할때마다 관련 정보를 넘겨주어야 해서 프론트에서 처리하는게 맞다고 해서

앗.. 그렇군요 ㅠㅠ

말씀해주신 방식에서는 클라이언트에서 해야하는게 맞는 것 같아요.

image url 10개까지 등록 가능하기 때문에 환경별로 등록해주면 어떨까요?

https://developers.kakao.com/docs/latest/ko/kakaologin/prerequisite#kakao-login-redirect-uri

와.. 이 부분은 저도 못보고 지나간것 같네요..! 이 글을 바탕으로 한번 의논해보겠습니다. 감사합니다!

@JunilHwang
Copy link

image

안녕하세요~
일단 로그인을 했을 때 refresh_token 자체를 안 내려주는데, 이 이후로는 제가 확인이 불가능하네요 ㅠㅠ
토큰이 내려오질 않아서요..

@JunilHwang
Copy link

일단 이 PR은 머지하겠습니다!

@JunilHwang JunilHwang merged commit 74faf7a into Review Oct 28, 2024
@Dobbymin
Copy link
Contributor Author

image 안녕하세요~ 일단 로그인을 했을 때 refresh_token 자체를 안 내려주는데, 이 이후로는 제가 확인이 불가능하네요 ㅠㅠ 토큰이 내려오질 않아서요..

다시 수정해보겠습니다 ㅠㅠ 감사합니다 멘토님!

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.

4 participants