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

feat(login): 로그인 화면에서 로그인 기능을 구현하고 정상 로그인 시 홈 페이지로 이동되도록 합니다. #151

Merged
merged 16 commits into from
Oct 5, 2023

Conversation

innocarpe
Copy link
Collaborator

📌 이슈 링크


📖 작업 배경


🛠️ 구현 내용

  • 회원 가입과 로그인에 둘 다 사용되는 응답 CurrentUser 를 새 타입으로 선언하고 기존 타입을 제거합니다.
  • LoginService 와 useLogin 을 작성합니다. 훅에는 로그인 성공시 JWT 토큰을 갱신하는 로직도 포함되어 있습니다.
  • 로그인 폼에서 로그인 상태를 전달받아 그에 따른 버튼 상태 변경 등을 UI를 개선했습니다.
  • 로그인 버튼을 눌러 로그인을 시도할 때, 실제로 로그인을 진행하도록 합니다. (로그인 페이지 컨테이너에서 처리)
  • 로그인에 성공하면 홈 화면으로 리다이렉트되도록 합니다. 기본 쿼리 파라미터를 붙여줄까 했지만 어차피 홈에서 알아서 처리를 해 주기에 그냥 Path 만 지정했습니다.
  • 로그인에 실패할 때에는 그 이유를 할 수 있도록 폼 쪽에 간단히 에러 메시지 컴포넌트를 붙여 에러를 표시하도록 해 주었습니다.

💡 참고사항

  • 최근 드는 고민인데, login.tsx 에서는 아무 역할도 하지 않고 LoginContainer 에서 모든 역할을 다 하고 있는데, 지금 생각해보면 컨테이너에서는 렌더링에 관련된 역할까지만 하고 최상위 컴포넌트에서 비즈니스 로직과 관련된 모든 처리를 하는게 맞는 방향인건가? 하는 생각이 드네요. 이건 기회가 될 때 다른 개발자 분들께 Best Practice 를 여쭤 보려고 합니다.

🖼️ 스크린샷

2023-09-24.12.52.28.mov

@innocarpe innocarpe added feature A new feature status: dependent 이전 PR의 커밋이 포함되어 있어 대기해야 함 labels Sep 23, 2023
Comment on lines 93 to 97
interface LoginButtonProps {
disabled: boolean;
children?: ReactNode;
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
interface LoginButtonProps {
disabled: boolean;
children?: ReactNode;
}
interface LoginButtonProps extends PropsWithChildren<{disabled: boolean}> {}

확장 가능성을 주지 않기 위해 Button 속성을 주지 않은 것일까요?
그렇다면 children은 기존에 있는 타입을 활용해주는 것이 소통하기 편하답니다!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@InSeong-So Button 속성으로 구현하는 방향이 있었군요..! 아래처럼 구현했습니다!

interface LoginButtonProps
  extends React.ButtonHTMLAttributes<HTMLButtonElement> {
  children?: ReactNode;
}
const LoginButton = (props: LoginButtonProps) => {
  const { disabled = false, children, ...restProps } = props;

  return (
    <StyledLoginButton
      type="submit"
      className={`btn btn-lg pull-xs-right`}
      disabled={disabled}
      {...restProps}
    >
      {children}
    </StyledLoginButton>
  );
};

Comment on lines +8 to +15
const { loginError, loginStatus, handleLogin } = useLogin();
const navigate = useNavigate();

useEffect(() => {
if (loginStatus === 'success') {
navigate('/'); // 홈페이지로 리다이렉트
}
}, [loginStatus, navigate]);
Copy link
Member

Choose a reason for hiding this comment

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

로그인 여부에 따라 리다이렉트라면 useLogin에서 하나의 함수로 묶어줘도 좋을 것 같아요. 동일한 관심사지 않나 싶습니다! handleValidAccess 라는 네이밍은 마음에 안 들지만, 이런 형태로 useLogin이 핸들링하는 것은 어떻게 생각하시나요?

Suggested change
const { loginError, loginStatus, handleLogin } = useLogin();
const navigate = useNavigate();
useEffect(() => {
if (loginStatus === 'success') {
navigate('/'); // 홈페이지로 리다이렉트
}
}, [loginStatus, navigate]);
const { loginError, loginStatus, handleValidAccess, handleLogin } = useLogin();
handleValidAccess();

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

음 이 부분은 코드 수정하면서 고민을 계속 해 보았는데, 제 선호이기는 하지만 저는 현재의 형태가 더 좋지 않을까 생각이 듭니다..! 아래 사항을 고려하였어요.

1. 책임 분리 원칙 (Separation of Concerns):

useLogin 훅을 최초 구현했던 의도는 로그인 기능에 대한 책임이었습니다. (이름도 이걸 반영하기도 했구요)
리다이렉트는 로그인 후의 화면 플로우(=네비게이션)와 관련된 부분이므로, 컴포넌트나 다른 훅에서 처리하는 것이 더 좋지 않을까 싶었어요.
그리고 추후에 로그인이 필요한 다른 페이지나 모달에서 useLogin 훅을 사용할 경우 리다이렉트 '/' 가 아닌 곳으로 처리해야 할 수도 있을 것 같기도 합니다.

2. 재사용성과 확장성:

추후에 로그인 로직이 변경되어도 리다이렉트와 관련된 코드는 영향을 받지 않도록 하고 싶습니다.
또한, 다른 로그인 방식 (예: 소셜 로그인)이 추가되더라도 기존의 로그인 훅을 재사용하기 쉽도록 만드는 방향이 좋지 않을까 싶었습니다. (최소한의 로직만 담기는)
대신에 로그인 성공 여부나 상태를 반환해 이를 사용하는 컴포넌트에서 리다이렉트를 처리하는 것이 좋을 것이라 생각했습니다!

Comment on lines 9 to 44
const [loginError, setLoginError] = useState<LoginUserErrors | null>(null);
const [loginStatus, setLoginStatus] = useState<LoginStatus>('idle');

const handleLogin = async (data: { email: string; password: string }) => {
try {
Copy link
Member

Choose a reason for hiding this comment

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

여기에 사용된 방식이 reducer 패턴인데요, 한 번 참고해보시면 좋을 것 같아요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

iOS 에서 늘 사용해오고 있던 방식이라 정말 반가운 방식이네요 ㅎㅎ 링크와 이것저것 참고해서 아래와 같이 고쳐보았어요!

export type LoginStatus = 'idle' | 'loggingIn' | 'success' | 'failed';

type LoginState = {
  loginError: LoginUserErrors | null;
  loginStatus: LoginStatus;
};

type LoginAction =
  | { type: 'LOGIN_START' }
  | { type: 'LOGIN_SUCCESS'; token: string }
  | { type: 'LOGIN_ERROR'; error: LoginUserErrors }
  | { type: 'LOGIN_FAILED' };

const loginReducer = (state: LoginState, action: LoginAction): LoginState => {
  switch (action.type) {
    case 'LOGIN_START':
      return { ...state, loginStatus: 'loggingIn', loginError: null };
    case 'LOGIN_SUCCESS':
      return { ...state, loginStatus: 'success', loginError: null };
    case 'LOGIN_ERROR':
      return { ...state, loginStatus: 'failed', loginError: action.error };
    case 'LOGIN_FAILED':
      return { ...state, loginStatus: 'failed' };
    default:
      return state;
  }
};

const useLogin = () => {
  const [state, dispatch] = useReducer(loginReducer, {
    loginError: null,
    loginStatus: 'idle',
  });

  const handleLogin = async (data: UserCredentials) => {
    dispatch({ type: 'LOGIN_START' });

    try {
      const response = await LoginService.loginUser(data);
      // JWT 토큰을 쿠키에 저장
      if (response && response.user && response.user.token) {
        saveTokenToCookie(response.user.token);
        dispatch({ type: 'LOGIN_SUCCESS', token: response.user.token });
        console.log('login suceess');
      }
      return response;
    } catch (error) {
      if (error && typeof error === 'object' && 'errors' in error) {
        dispatch({
          type: 'LOGIN_ERROR',
          error: error.errors as LoginUserErrors,
        });
        console.error('LoginUserErrors: ', error.errors);
      } else {
        dispatch({ type: 'LOGIN_FAILED' });
        console.error('An unexpected error occurred:', error);
      }
      return null;
    }
  };

  return {
    loginError: state.loginError,
    loginStatus: state.loginStatus,
    handleLogin,
  };
};

Base automatically changed from carpe/revise-login-form to team6/innocarpe October 5, 2023 12:03
@innocarpe innocarpe removed the status: dependent 이전 PR의 커밋이 포함되어 있어 대기해야 함 label Oct 5, 2023
@innocarpe innocarpe force-pushed the carpe/implement-login-api branch 2 times, most recently from 94c3cae to 904b0c5 Compare October 5, 2023 13:56
@innocarpe innocarpe merged commit afcffec into team6/innocarpe Oct 5, 2023
@innocarpe innocarpe deleted the carpe/implement-login-api branch October 5, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants