-
Notifications
You must be signed in to change notification settings - Fork 44
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
[김법균] Week 15 #482
The head ref may contain hidden characters: "part3-\uAE40\uBC95\uADE0-week15"
[김법균] Week 15 #482
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.
법균님! 고생 많으셨습니다 ㅎㅎ 많이 고민한게 보이는 코드에요.
피드 달아주신 내용 우선 같이 봐볼게요.
지난 피드백중 미반영 사항들 위주 내용들입니다
- js 파일이름 컨벤션관련 권유해주신 링크에선 스타일시트 관련 네이밍컨벤션이 안보였고, 컴포넌트 이름대로 모듈 css를 관리하는것도 흔한 컨벤션이라고해서 바꾸지 않았습니다.
- 네 좋습니다!
- 다만 이는 콤포넌트 파일 (jsx)와 스타일 시트 (module.css)에 대해서만 반영되는 내용으로 판단이 되는데요
- 현 프로젝트의 경우, 콤포넌트와 스타일 시트를 담고 있는 폴더 명들도 Pascal을 사용하고 있어서요.
- 이 부분에 대해서는 어떻게 생각하고 계시는지 읙녀을 여쭙고 싶어요.
- Account 컴포넌트 이름 바꾸는걸 깜빡하긴 했는데 user-profile로 갈 수 없었던 이유가 저 컴포넌트는 현재 사용자의 정보를 담는 것이 아닌 폴더 생성자의 정보를 담는것이라서 user를 컴포넌트 이름으로사용하지 않았습니다.
- Account라는 컴포넌트 이름의 경우, 로그인 한 사용자의 고객 정보를 보여주는 요소를 보통 어카운트라고 이야기를 주로 하기는 합니다. 계정 정보라는 뜻을 담고 있기도 하고, 계정 정보는 로그인 한 사용자나 어드민 이용자가 아니면 볼 수 없어야 하는 요소이기도 하지요.
- Account 컴포넌트에선 불필요한 정보를 받지 않기위해 타입선언을 따로 하지 않았습니다.
- 👍🏻
- 주체와 제어권의 역전은 일단 영문서를 조금 보다 말아서 반영을 못했습니다.
- 이 부분은 계속해서 고민해나가야 하는 부분이기도 합니다 ㅎㅎ
- 저번 멘토링 때 이야기 했던 내용 한번 참고해서 계속 학습해나가시는걸 추천드려요!
- 상수화 관련 부분은 예전에 일명 하드코딩 관련 피드백을 받은 이후 값들을 전부 상수로 빼는 작업이 한 번 있었습니다.
- 사실 이것도 항상 딜레마이긴 한데요
- 어느정도 범위까지 상수화를 할 것인가가 항상 큰 논쟁거리입니다.
- 개인적으로, 딱 한곳에서밖에 활용되지 않는 경우에는 굳이 상수화를 하지 않는 편이기는 합니다. 프로젝트 복잡도를 높이는 요소로 동작하기도 해서요.
- 이에 대해서는 법균님이 코드 작성하시면서 하나의 규칙으로 한번 구조화 해 나가봐도 좋겠네요
질문사항
- 코드를 최대한 분리를 한다고 하긴 해봤는데 이게 맞는 방향인지 감이 안옵니다.
- 좋은 방향으로 가고 있어요 ㅎㅎ
- 사실 완벽한 코드 분리 방법은 존재하지 않아요. 이거 재미있게 표현한게 ㅋㅋ 코딩애플이라는 아저씨가 릴스로 만든게 있는데 한번 봐보시면 재미있습니다.
- 그래서 가장 확실한 방향은 지속적으로 고민을 하면서 내가 봤을때 내 코드가 쓰기 쉽다 느껴질떄까지 계속 리팩토링을 해나가는것밖에 없긴 합니다.
- 그러면서 이게 좋은 방향인지 참조하기 위해 오픈소스 프로젝트나 전문 서적등을 참고하고, 디자인 패턴과 클린코드 관점에서 지속적인 학습을 해 나가시면 될 거에요.
- 라우팅때문에 사실상 서버사이드 렌더링을 쓸수 있는 페이지가 매우 제한적이었는데 이건 제가 최상단을 클라이언트사이드 렌더링을 해서 그런것같습니다. 이쪽관련 분해후 재편성은 나중에 해보겠습니다...
- 👍🏻
이 외에 자세한 내용들은 코드단 리뷰 남겨두었으니 참고 부탁드려요!
고생 많으셨어요
@@ -0,0 +1,41 @@ | |||
import z from 'zod'; | |||
|
|||
export const SignInFormSchema = z.object({ |
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.
schema 이름이 대문자일 필요는 없을 것 같아요!
|
||
export type SignInFormFields = z.infer<typeof SignInFormSchema>; | ||
|
||
export const SignUpFormSchema = z |
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.
마찬가지입니다!
params: { folderId: string }; | ||
} | ||
|
||
async function fetchFolderData(folderId: string) { |
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.
어차피 데이터를 반환하는 함수들이고,
데이터를 보관하는 변수라면
함수 또는 변수 이름에 data
라는 값이 굳이 붙여둘 필요가 있을까 싶어요
async function fetchFolderData(folderId: string) { | ||
const response = await axiosInstance.get(`/folders/${folderId}`); | ||
console.log(response); | ||
return response.data.data[0]; |
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.
또한 이처럼 반복적으로 진행되는 parsing 로직의 경우, axios에서 제공해주는 interceptor를 활용해 공통 처리를 해볼 수 있을 것 같아요
|
||
export default function SignUp() { | ||
const router = useRouter(); | ||
const { user } = useUserInfo(); |
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.
서버 세션에서 사용자 정보를 유지 / 관리 하도록 하는 형태도 고민해보면 좋겠어요!
순전히 userInfo를 위해 클라 콤포로 렌더되게 되면 ssr의 강점을 살리기 어렵잖아요 ㅎㅎ
const submitSignIn: SubmitHandler<SignInFormFields> = async (data) => { | ||
try { | ||
const response = await axiosInstance.post('/sign-in', data); | ||
if (response.status >= 200 && response.status < 300) { | ||
const accessToken = response.data.data.accessToken; | ||
localStorage.setItem('accessToken', accessToken); | ||
router.push('/folder'); | ||
} | ||
} catch (error) { | ||
throw new Error('로그인 실패'); | ||
} | ||
}; |
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.
로그인 폼에서 실행되는 로직과
auth context에서 기록 관리하는 로직이 분리가 되어야 할 것 같아요
로그인 폼에서야 말로 로그인을 위한 기능을 가져와 실행해야 할 거 같구
그에대한 결과에 따라 auth context에 저장된 auth 정보를 업데이트 하는 형태루요!
const submitSignUp = async (email: string, password: string) => { | ||
const response = await axiosInstance.post('/sign-up', { | ||
email, | ||
password, | ||
}); | ||
if (response.status < 400) { | ||
const accessToken = response.data.data.accessToken; | ||
localStorage.setItem('accessToken', accessToken); | ||
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.
@bk-git-hub
여기도 마찬가지에요!
); | ||
}; | ||
|
||
export const useAuth = () => useContext(AuthContext); |
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.
개발자들이 실수로 provider로 감쌎 ㅣ않은 곳에서 이 훅을 호출할 수 있을 것 같은데
방지를 위해 배리어를 한번 두면 어떨까요?
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.
auth context와 로그인 / 회원가입의 기능이 조금 섞인 것 같아요
auth context에서 기록 관리하고자 하는게 무엇인지 조금만 더 명확하게 굽분해보면 어떨까요?
const folderEndPoint = | ||
folderId && folderId !== -1 ? `?folderId=${folderId}` : ''; |
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.
쿼리 파람 설정의 경우 안전하게 URLSearchParam을 활용해주세요!
요구사항
배포링크
https://5-weekly-mission-eta-two.vercel.app/
기본
심화
주요 변경사항
멘토에게
지난 피드백중 미반영 사항들 위주 내용들입니다
질문사항