-
Notifications
You must be signed in to change notification settings - Fork 51
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
[전수빈] week15 #545
The head ref may contain hidden characters: "part3-\uC804\uC218\uBE48-week15"
[전수빈] week15 #545
Conversation
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. 인증관련된 코드는 인증 관련된 폴더에서만 관리한다.
우선 api/auth에서 인증관련 처리를 모아두고 있는것은 굉장히 잘하셨구요. 현재 폴더구조의 한계라고 볼 수 있는데, 동일한 관심사를 같은 폴더에 모아두고 보기는 어렵습니다. 제가 선호하는 폴더구조 기준으로 설명을 드리면 저는 인증 관련 코드를 아래처럼 모아둬요
lib
auth
api
login.ts
logout.ts
...
hooks
useUser.ts
useAccessToken.ts
useSession.ts
components
withAuthRequired.tsx
AuthRequired.tsx
인증이 필요한 페이지에는 아예 렌더링 자체가 안되게 component로 제어를 하겠죠
import {withAuthRequired} from 'libs/auth/components';
function SensitivePage() {
const user = useUser();
return <>{...}</>
}
// HOC로 렌더링 자체를 막아버리기
export default withAuthRequired(SensitivePage);
인증이 될때랑 안됐을때랑 일부 영역만 다르게 표현해야 한다면 이렇게 할 것 같네요
import {useUser} from 'libs/auth/hooks';
function Header() {
const user = useUser();
return (
<>
{user ? <UserInfo/> : <Login/>}
</>
);
}
조금 더 깔끔한 솔루션은 역시 컴포넌트를 이용하는 것이겠죠
import {AuthRequired} from 'libs/auth/components';
function Header() {
return (
<>
{<AuthRequired fallback={<Login/>}><UserInfo/></AuthRequired>}
</>
);
}
현재의 폴더구조대로 한다면 아래처럼 폴더구조가 나와주어야 할것같네요
components
auth
api
auth
hooks
auth
2. 토큰 관리는 서버에서 한다.
멘토링때도 말씀드렸지만, 과제 범위를 떠나서 실질적으로는 localStorage가 아닌 httpOnly, secure cookie를 이용하여 토큰 관리를 서버에서 하는 것이 좋습니다.
3. 다른 인증 라이브러리들의 사용법을 참고해 본다.
관심사를 분리하라고 했을 때, 사실 관심사가 뭔지 잘 감이 안오실 텐데요, 그럴때는 다른 인증라이브러리들이 어떤 기능을 제공하고 있는지 참고해보시면 좋습니다.
일반적으로는 토큰/세션 관리, 리다이렉트 처리 등등의 기능이 있다고 보시면 되구요, 위의 기능을 인증레이어가 아니라 직접 구현하고 있는 컴포넌트가 있다면 인증관련 구현 디테일이 새어나갔고, 리팩토링이 필요하다고 보시면 됩니다.
password: string; | ||
} | ||
|
||
interface SaveTokenProps { |
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.
Props라는 이름은 properties
를 축약한 것으로 react에서 html properties스펙에 맞게 지은 이름입니다.
이 타입은 html props가 아니니까 굳이 props라는 이름을 지을 필요가 없습니다. 차라리 PersistedTokens
같은 이름이 좋을 것 같아요
} | ||
throw new Error(); | ||
} catch (e) { | ||
return 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.
멘토링 시간때 에러핸들링 관련해서 말씀드렸던것처럼 이 함수에서는 차라리 error를 catch하지 않는 것이 좋습니다.
|
||
if (response.status === 200) { | ||
const { data } = response; | ||
saveToken(data.data); |
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.
인증 관련 파일 내부에서 token관리를 하고 계시네요 관심사에 맞게 잘 하고 계십니다. 👍
if (result.status === 200) { | ||
const { data } = result; | ||
localStorage.setItem("accessToken", data.data.accessToken); | ||
if (result) { | ||
router.push("/folder"); |
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.
localStorage관련 코드가 사라져서 훨씬 깔끔하네요.
redirect도 auth 관련 코드 내부에서 해주면 어떨까요?
signUp(data, {redirectTo: '/folder'});
@@ -25,10 +24,13 @@ const Header = () => { | |||
id: null, | |||
image_source: "", | |||
}); | |||
const [accessToken, setAccessToken] = useState(""); |
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.
accessToken관련 상태도 auth 라는 도메인의 상태로 옮겨주시는것이 좋을것 같네요
const {isLoggedIn} = useAuth();
return <>{isLoggedIn ? <UserInfo/> : <LoginButton/>}</>
요구사항
기본
심화
주요 변경사항
스크린샷
2023-12-17.11.54.59.mov
2023-12-17.11.57.14.mov
멘토에게