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

[Park] Firebase Auth 연동 #51

Merged
merged 16 commits into from
Oct 13, 2022
Merged

[Park] Firebase Auth 연동 #51

merged 16 commits into from
Oct 13, 2022

Conversation

healtheloper
Copy link
Member

@healtheloper healtheloper commented Oct 11, 2022

close #49

  • ) 환경변수 업데이트되어서 내일 스크럼에서 말씀드릴게요
  • ) netlify 는 github application 에서 callback url 을 PR deploy 도메인을 설정하지 않아서 login 을 확인하기가 어려운데
    저렇게 동적으로 변하는 url 을 어떻게 설정해야 하는지도 알아보겠습니다..

Actions

  • firebase 를 위한 env 변동사항이 있어 workflow 의 yml 파일 수정도 있습니다
  • 참고

dependency

  • Firebae 가 현재 v9.11.0 을 사용하고 있는데 dependency 관련 이슈가 있어서 Downgrade 를 권한 상태입니다.
    이번 주에 개선이 될 예정이라고 하니 개선된 이후 버전 업그레이드 진행하겠습니다.
  • git action 시에 msw 의 dependency 인 header-polyfills 관련 오류가 있어 msw 에서 이를 수정한 버전을 바탕으로 패키지 업그레이드랬습니다.

@fbase/auth

유저와 관련된 메서드는 @fbase/auth 에서 확인하실 수 있고, 만약 필요하시다면 firebase 공식문서를 통해 직접 추가도 가능합니다.
학습 비용이 있어 필요하신 부분이 있으시면 웬만하면 제가 하긴 하겠습니다!

firebase 를 연동하면서 user 의 정보를 받아오는 부분에 이슈가 살짝 있었는데,
제가 알고 있던 것은 auth.currentUser 하면 바로 로그인된 유저의 정보를 받아와서 사용하는데 Page reload 시에 가져오지 못하는 문제가 있었습니다.

구글링해본 결과 User 를 받아오는 것이 Promise 가 아니기에 onAuthChanged 라는 listener 를 통해 User 를 받는 것을 등록해놓고 사용하기를 권장하고 있어 useMe 라는 훅을 만들어 useEffect 에서 상태를 통해 set 하도록 하였습니다.

여기서 또 문제가 발생한 점은 아무래도 User 정보를 받아오는데 딜레이가 있기 때문에
App.tsx 단게에서 local storage 에 이미 로그인을 한 이력이 있는지 판단하는 isLogin 상태를 사용하여 isLogin 인데 user 의 정보가 없는 경우 렌더링을 하지 않도록 하여 반짝이는 현상을 없앴습니다.

Oct-12-2022 01-57-42

+) 만약 token 이 만료되어서 user 를 받아오지 못한거라면 이슈가 생길 수가 있을 것 같은데 해당 부분에 대한 테스트는 추후 진행해보겠습니다.

++) 위와 관련해서 기존에 셋팅해두었던 ErrorBoundary 의 로직도 수정될 필요가 있어보이고, 전역 상태를 관리하는 Recoil Store 에서 user 정보가 불필요하다고 생각이 되어 추후 없애지 않을까 생각중입니다.

@netlify
Copy link

netlify bot commented Oct 11, 2022

Deploy Preview for co-studo ready!

Name Link
🔨 Latest commit 7302bfa
🔍 Latest deploy log https://app.netlify.com/sites/co-studo/deploys/634669678bf4c900094ff6b1
😎 Deploy Preview https://deploy-preview-51--co-studo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Contributor

@hemudi hemudi left a comment

Choose a reason for hiding this comment

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

늦은 리뷰 죄송합니다...!
전체적으로 코드를 훑어봤는데 깔끔해서 그런가 술술 읽혔네요!
LGTM 입니다 ^_^)b

firebase 작업해주셔서 넘 감사하고
저도 틈틈히 관심을 가지고 학습해서 도움이 되도록 노력하겠습니다! ^_^)7
고생하셨습니다! 팤흐!

Comment on lines +40 to +51
export const useMe = () => {
const [me, setMe] = useState<User | null>(null);

useEffect(() => {
const unsubscribe = auth.onAuthStateChanged((user) => {
setMe(user);
});
return () => unsubscribe();
}, []);

return me;
};
Copy link
Contributor

@hemudi hemudi Oct 13, 2022

Choose a reason for hiding this comment

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

useMe라는 네이밍이 뭔가 엄청 귀여운데
이름만 봤을 때 user의 정보를 가져오는 느낌이 직관적으로 들지는 않는 것 같아요
근데 너무 귀엽네요 useMe...setMe...ㅋㅋㅋㅋㅋㅋㅋㅋㅋㅋ^_^)b 오히려 좋은 것 같기도?ㅋㅋㅋㅋ

Copy link
Member Author

Choose a reason for hiding this comment

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

사용한다 나를..!

현재 로그인된 사람의 정보가 필요한데 useLoginUser ? 정도가 떠오르긴 하네용 ㅋㅋ
다른 분들도 useMe 라고 하면 동작이 예상이 어려울까요?

Copy link
Member

Choose a reason for hiding this comment

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

흠 좀 더 구체적이면 좋을 것 같긴 한데 useMe도 나쁘지는 않은 것 같아요..!

Copy link
Contributor

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋ 귀엽긴하네용 나를 사용하라..!
근데 첨에 보고 useMe ?? 나의 뭘 사용한다는거지?? 라는 생각이 들어서 이 파일을 열어서 확인하긴 했어요ㅎㅎ
useLoginUser가 더 명확하고 좋을거 같아요!

Copy link
Member

@mina-gwak mina-gwak left a comment

Choose a reason for hiding this comment

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

오호 firebase에서 자체적으로 제공해주는 기능이 많네요!
저도 시간 나면 학습해볼 수 있도록 하겠습니당

고생하셨어요 파크!!


const auth = getAuth(app);

export const login = ({ email, password, authProvider }: LoginInput) => {
Copy link
Member

Choose a reason for hiding this comment

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

보통 저희가 props 네이밍을 XXXProps 라고 했던 것 같은데 LoginInput이라는 이름을 사용한 이유가 있으실까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

고민했던 부분이긴 한데 Props 의 의미가 컴포넌트에서 쓰이는게 맞다고 생각해서 login 은 login 을 제공하는 함수이기에 네이밍을 저렇게 지었습니다

Copy link
Member

Choose a reason for hiding this comment

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

아하 그렇군여! 그런데 input이라고 하면 딱 생각나는 게 폼에서 사용하는 input이라 더 나은 이름이 있을지 고민해봐도 좋을 것 같네요!

Copy link
Contributor

Choose a reason for hiding this comment

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

움.. 근데 사용자가 입력한 값이니 (물론 지금은 github 로그인이라 사용자가 직접 입력하진 않았지만) input도 저는 괜찮은거 같아요! 저라면 LoginInfo 라고 지었을거 같긴해요.

Copy link
Member Author

Choose a reason for hiding this comment

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

음.. 일단은 emai, password 모두 input 을 통해 받는 값이긴 하니 그대로 두겠습니다!


const auth = getAuth(app);

export const login = ({ email, password, authProvider }: LoginInput) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

움.. 근데 사용자가 입력한 값이니 (물론 지금은 github 로그인이라 사용자가 직접 입력하진 않았지만) input도 저는 괜찮은거 같아요! 저라면 LoginInfo 라고 지었을거 같긴해요.

Comment on lines +40 to +51
export const useMe = () => {
const [me, setMe] = useState<User | null>(null);

useEffect(() => {
const unsubscribe = auth.onAuthStateChanged((user) => {
setMe(user);
});
return () => unsubscribe();
}, []);

return me;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

ㅋㅋㅋㅋㅋㅋ 귀엽긴하네용 나를 사용하라..!
근데 첨에 보고 useMe ?? 나의 뭘 사용한다는거지?? 라는 생각이 들어서 이 파일을 열어서 확인하긴 했어요ㅎㅎ
useLoginUser가 더 명확하고 좋을거 같아요!

setMe(user);
});
return () => unsubscribe();
}, []);
Copy link
Contributor

Choose a reason for hiding this comment

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

unmount되면 유저 정보를 없애는? 로직 같은데 onAuthStateChanged는 유저 정보가 변할 때 발생하는 이벤트니 subscirbe와 unsubscribe 둘 다 해당되는 기능인거 같아요.
unsubscribe 이름대로라면

    const unsubscribe = auth.onAuthStateChanged((user) => {
      setMe(null);
    });
    return () => unsubscribe();

이렇게 되야할거 같은데 지금 이 함수만 보면 user 정보가 들어올 때도 해당되는 함수 아닌가? 라는 생각이 들어서요!
지금 로직 그대로 쓰려면

   const updateLoginUser = auth.onAuthStateChanged((user) => {
      setMe(user);
    });
    return () => updateLoginUser();

이런 이름이 되야하지 저 함수의 기능에 맞지 않나라는 생각이 드네요..??
unmount될 때 로그인 정보를 해지한다라는걸 알려주기 위해서 사용하신 거 같은데 함수 기능에는 맞지 않는 이름 아닌가라는 생각이 듭니당

Copy link
Member Author

Choose a reason for hiding this comment

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

onAuthStateChanged 의 return 값이 unsubscribe 함수여서 저렇게 사용했습니다!

https://firebase.google.com/docs/reference/js/v8/firebase.auth.Auth#onauthstatechanged

로그인 정보를 해지한다 라는 것 보다는 user 의 state change 를 감지하는 listener 를 해지하는 것으로 생각하시면 됩니다!

const [isLogin] = useLocalStorage('isLogin', false);
const user = useMe();

return isLogin && !user ? null : (
Copy link
Contributor

Choose a reason for hiding this comment

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

추후에 로딩 컴포넌트를 만들어서 null 대신 넣으면 좋을거 같아용

@@ -0,0 +1,32 @@
import { brands } from '@fortawesome/fontawesome-svg-core/import.macro';
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome';
Copy link
Contributor

Choose a reason for hiding this comment

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

말하는걸 계속 깜빡했는데 @FortAwesome 요기 오타가 있더라구용 fort -> font

Copy link
Member Author

Choose a reason for hiding this comment

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

저거 패키지 이름이 저렇습니다 ㅋㅋ; 아마 누가 먼저 선점해서 저렇게 썼나봐요

@jindonyy
Copy link
Contributor

firebase 잘 정복해가고 계신거 같네요!
수고하셨습니다 파크!!! 👍

@healtheloper healtheloper merged commit ff0b46f into dev Oct 13, 2022
@healtheloper healtheloper deleted the feat/firebase-auth/#49 branch October 13, 2022 09:17
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.

FIrebase Auth 부분 도입
4 participants