[feat] OAuth SignIn 구현 #42

Kakamotobi · 2023-10-23T07:17:08Z

구현한 것

자세한 내용은 아래 이슈를 참고해주세요.
[feat] SignUp 및 SignIn 구현 #4

기타

bakhacode · 2023-10-23T07:37:26Z

fe/src/components/auth/KakaoSignInButton.tsx

+  const onClick = () => {
+    const oAuthPopUpWindow = openPopUpWindow(
+      `https://kauth.kakao.com/oauth/authorize?response_type=code&client_id=${
+        import.meta.env.VITE_KAKAO_CLIENT_ID
+      }&redirect_uri=${CLIENT_URL}/signin?provider=kakao`,
+      "kakaoOAuth",
+      500,
+      600
+    )!; // TODO: handle case where popup doesn't show (Ex: user blocked popups)
+
+    onOpenPopUpWindow(oAuthPopUpWindow);
+  };


onClick보다 뭔가 눌렀을 때 openPopUpWindow하는 동작을 설명해줄 수 있는 함수명 없을까요? 🤔 GPT한테 물어봤을 때는 openKakaoOAuthPopup를 추천해주긴하는데 어떨까요?

openKakaoSignInPopUp이 괜찮을것 같은데, on prefix를 지키려면 onKakaoSignIn, onKakaoSignInBtnClick, onKakaoPopUpOpen, onKakaoSignInPopUpOpen 등이 있을 것 같네요. 박하는 어떤게 더 적절한 것 같으신가요? 😵‍💫

onKakaoSigIn이 제일 나아보이네요 !

bakhacode · 2023-10-23T07:42:09Z

fe/src/pages/SignInPage.tsx

+    };
+  }, [closePopUpWindow, oAuthSignInMutate, popUpWindow]);
+
+  const isAllFieldsFilled = !!email && !emailError && !!password;


rhyme 좋네요 🎶

altmit · 2023-10-23T07:42:00Z

fe/src/components/auth/KakaoSignInButton.tsx

+    const oAuthPopUpWindow = openPopUpWindow(
+      `https://kauth.kakao.com/oauth/authorize?response_type=code&client_id=${
+        import.meta.env.VITE_KAKAO_CLIENT_ID
+      }&redirect_uri=${CLIENT_URL}/signin?provider=kakao`,
+      "kakaoOAuth",
+      500,
+      600
+    )!; // TODO: handle case where popup doesn't show (Ex: user blocked popups)


우선 코드를 직접 확인하지는 않았지만 ! 를 추가하신 이유는 openPopUpWindow가 null이나 undefined를 return 할 수 있어서 인가요?

네 openPopUpWindow 내부에서 window.open()을 하고 있는데 반환값이 Window | null입니다.
!보다 if로 type guard 적용하는게 나을 것 같네요

altmit · 2023-10-23T07:45:37Z

fe/src/components/auth/NaverSignInButton.tsx

+    clientId: import.meta.env.VITE_NAVER_CLIENT_ID,
+    callbackUrl: `${import.meta.env.VITE_CLIENT_URL}/signin?provider=naver`,


env 값들도 상수로 관리해서 조금 짧게 사용하면 좋을 것 같아요

altmit · 2023-10-23T07:49:30Z

fe/src/context/WindowContext.tsx

+});
+
+export function WindowProvider({ children }: { children: ReactNode }) {
+  const [popUpWindow, setPopUpWindow] = useState<Window | null>(null);


const [popUpWindow, setPopUpWindow] = useState<Window>();

이렇게 초기값을 안주고 undefined로 사용할 수도 있는데 null을 사용하신 이유가 undefined와 null의 영어적 의미나 뜻 때문인가요?

undefined의 정의("변수가 선언은 되었지만 아직 값을 할당받지 않았다")를 보았을 때는 undefined가 적절한 것 같지만, popup window를 닫을 때 setPopUpWindow(undefined)는 어색한 것 같아요. 저는 개인적으로 undefined는 처음에 변수를 선언할 때는 빈 값으로서 괜찮지만 후에 값이 생겼다가 없애는 경우에는 null("값이 없다")이 더 적절한 것 같아서 null을 사용했습니다.

https://stackoverflow.com/a/57249968/15330887

저도 setPopUpWindow(undefined) 처럼 undefined로 set하는다는게 조금 어색하게 느껴져서 물어본 질문 이였습니다👍

altmit · 2023-10-23T08:04:14Z

fe/src/pages/SignInPage.tsx

+  const {
+    value: email,
+    error: emailError,
+    onChange: onEmailChange,
+  } = useText({ validators: [validateEmail] });


이번 코드처럼 구조 분해 할당을 1개만 하는 경우는 괜찮은데 여러개 사용하는 경우에는 모두 구조 분해 할당하며 다시 이름 지정하는게 귀찮기도하고 조금 복잡해 보여서 저는 아래처럼 사용하는데 어떻게 생각하시나요.

대신 사용하는 곳이 조금 길어지긴 합니다.

const nickname = useText({ validators: [validateNickname], }); const password = useText({ validators: [validatePassword], }); // 사용하는 곳 password.onChange nickname.value

개인적으로 사용하는 곳에서 .으로 체이닝을 계속 하는게 더 불편하고 코드가 길어지기 때문에 읽기 어려운것 같아요. 같이 convention을 정하면 좋을 것 같습니다 👍

저는 사용하는 곳에서 짧은게 좋은 것 같아요

지금 처럼 체이닝이 단 한번이라면 선언에서 리네이밍 하는 것 보다 사용처에서 자동완성으로 딱딱 쓰는게 편하다 생각했는데 두 분은 아니군요😥

<Input placeholder="닉네임" value={nickname.value} onChange={(e) => nickname.onChange(e.target.value.trim())} />

단점으로는 onChage의 경우 이렇게 길어지긴 합니다.
이 단점을 없애고 싶어서 아래에 e.target.value.trim() 관련 질문이 연결된 질문이기도 합니다😂

그렇군요 😅 그러면 체이닝 안하는 방식으로 진행해도 괜찮으신걸로 이해해도 될까요?

useText 관련해서 사용해봤을 때 구조 분해 할당 해놓은 값들을 엄청 자주 쓰진 않아서 크게 불편하진 않았던 거 같아서 괜찮게 느끼는 것 같기도?

altmit · 2023-10-23T08:10:54Z

fe/src/pages/SignInPage.tsx

+            type="text"
+            placeholder="이메일"
+            value={email}
+            onChange={(e) => onEmailChange(e.target.value.trim())}


input에 onChage에서 e.target.value.trim()를 사용하지 않고 다른 값을 전달해야 하는 경우가 있나요?

그렇지 않다면 위 로직을 useText가 하게 하면 조금 더 짧게 사용 가능할 수 있을 것 같네요

onChange={onEmailChange}

useText를 사용하지만 trim()을 하고 싶지 않을 수도 있기 때문에 onChange는 단순히 string만 받게 했습니다. 사용처에서 원하는대로 값을 조작하는 방식으로 하는게 나을 것 같다고 생각했습니다.

다시 생각해 보니 기본 로직 onChange에 trim()을 하면 공백을 넣을 수 없게 되겠네요😂

altmit · 2023-10-23T08:11:32Z

fe/src/pages/SignInPage.tsx

+            value={email}
+            onChange={(e) => onEmailChange(e.target.value.trim())}
+          />
+          {<TextInputError>{emailError}</TextInputError>}


{} 없어도 괜찮지 않나요?

altmit · 2023-10-23T08:14:45Z

fe/src/pages/SignUpPage/SignUpPage.tsx

+            onNext={(password: string, passwordConfirm: string) => {
+              setSignUpData((prev) => ({ ...prev, password, passwordConfirm }));
              setSubPage("verification");
+              // Request server to send verification code
+              postEmailVerification(signUpData.email);
            }}


짧을 때는 그냥 볼 수 있을 것 같은데 조금 늘어서 그런지 onNext에 들어갈 함수를 따로 빼는것도 좋을 것 같네요.

전체적으로 리팩토링이 필요한 부분입니다

Kakamotobi added 15 commits October 19, 2023 12:28

#4 feat: Sign up data에 passwordConfirm field 추가

20618fa

#4 feat: Sign up, 닉네임/이메일 중복검사 api 및 mock 추가

39aee66

#4 fix: useText 초기값 validate 적용

eecf64c

#4 feat: NicknameSubPage "다음" 버튼 disabled 조건 적용

d255467

#4 feat: Email verification code api 및 mock 추가

bfbe37f

#4 feat: Sign up subpage "다음" 버튼 disabled 조건 추가

15228c8

#4 feat: Signup nickname 중복 체크 기능 추가

9ff9762

#4 feat: Signup email 중복 체크 기능 추가

6e8cef7

#4 feat: Signup password confirm mismatch 에러 메시지 추가

2d85e91

#4 feat: Signup email verification code 요청 추가

d009390

#4 feat: Google SignIn 추가

0ec3377

#4 feat: Popup window 구현

77bdfb7

#4 feat: Kakao 로그인 버튼 구현

2472c39

#4 feat: Naver 로그인 버튼 구현

d08a930

#4 style: console.log 제거

09f2f46

Kakamotobi added frontend 프론트 이슈 feat 새로운 기능 추가 labels Oct 23, 2023

Kakamotobi added this to the [FE] Sprint #2 milestone Oct 23, 2023

Kakamotobi requested review from altmit and bakhacode October 23, 2023 07:17

Kakamotobi self-assigned this Oct 23, 2023

Kakamotobi linked an issue Oct 23, 2023 that may be closed by this pull request

[feat] SignUp 및 SignIn 구현 #4

Closed

bakhacode reviewed Oct 23, 2023

View reviewed changes

#4 fix: Window.naver doesn't exist type error

47ee0f3

altmit approved these changes Oct 23, 2023

View reviewed changes

Kakamotobi added 3 commits October 23, 2023 17:26

#4 refactor: KakaoSignInButton oAuthPopUpWindow type guard 적용

dc4f126

#4 refactor: Env variables 상수화

a654e24

Merge branch 'dev-fe' into fe/feat/#4-signup-page

07a3f10

bakhacode merged commit 0cd75ad into dev-fe Oct 23, 2023

bakhacode deleted the fe/feat/#4-signup-page branch October 23, 2023 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[feat] OAuth SignIn 구현 #42

[feat] OAuth SignIn 구현 #42

Kakamotobi commented Oct 23, 2023

bakhacode Oct 23, 2023

Kakamotobi Oct 23, 2023

bakhacode Oct 23, 2023 •

edited

Loading

bakhacode Oct 23, 2023

altmit Oct 23, 2023

Kakamotobi Oct 23, 2023

altmit Oct 23, 2023

altmit Oct 23, 2023

Kakamotobi Oct 23, 2023

altmit Oct 23, 2023

altmit Oct 23, 2023

Kakamotobi Oct 23, 2023

bakhacode Oct 23, 2023

altmit Oct 23, 2023

Kakamotobi Oct 23, 2023

bakhacode Oct 23, 2023

altmit Oct 23, 2023

Kakamotobi Oct 23, 2023

altmit Oct 23, 2023

altmit Oct 23, 2023

altmit Oct 23, 2023

Kakamotobi Oct 23, 2023

		clientId: import.meta.env.VITE_NAVER_CLIENT_ID,
		callbackUrl: `${import.meta.env.VITE_CLIENT_URL}/signin?provider=naver`,

[feat] OAuth SignIn 구현 #42

[feat] OAuth SignIn 구현 #42

Conversation

Kakamotobi commented Oct 23, 2023

구현한 것

기타

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakhacode Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bakhacode Oct 23, 2023 •

edited

Loading