-
Notifications
You must be signed in to change notification settings - Fork 0
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] 유저 정보 관리 cookie로 변경 #368
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.
코멘트 완료했습니다 포메~!! 쿠키로 마이그레이션하느라 힘들었을 것 같은데 잘 바꿔주셔서 감사합니다 😁 몇가지 코멘트 남겨봤습니다 같이 얘기해봐요!!!!!!!
@@ -16,6 +16,7 @@ const fetcher = { | |||
method, | |||
body: body && JSON.stringify(body), | |||
headers: headers && headers, | |||
credentials: 'include', |
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.
💭 질문 💭
오호 이 속성을 붙여야 쿠키를 주고받을 수 있는건가요??
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.
네, 이 속성이 있어야 쿠키가 전송이 되더라고요!
frontend/src/apis/room.ts
Outdated
@@ -136,3 +142,12 @@ export const isJoinableRoom = async (roomUuid: string): Promise<{ isJoinable: bo | |||
|
|||
return data; | |||
}; | |||
|
|||
// 사용자 정보 조회 | |||
export const getMember = async (): Promise<RoomAndMember> => { |
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.
🙏 제안 🙏
getMember하면 뭔가 string 값이 반환될 것 같다고 느껴지는데 getUserInfo 이런 느낌은 어떤가요?? getMember는 객체로 반환하는데 객체 키값에 member가 있어서 어색하다고 느낀 것 같아요.
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.
네, 그렇게 수정하겠습니다!
} | ||
}, [master.memberId, memberInfo, setMemberInfo]); | ||
queryClient.invalidateQueries({ queryKey: ['getMember'] }); | ||
}, [master.memberId]); |
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.
🌸 칭찬 🌸
오 유저정보를 서버에서 관리하니까 해당 처리가 가능하군요!!! 될 것 같긴 한데 잘 동작하는지 영상이 있으면 더 이해가 빠를 것 같아용
@@ -11,7 +11,7 @@ export const spinnerWrapper = css` | |||
|
|||
export const rotatingImage = (size: number) => css` | |||
width: ${size}rem; | |||
height: 20vh; | |||
height: ${size * 2}rem; |
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.
💭 질문 💭
너비의 2배 높이 비율로 해도 괜찮았나요??
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대1로 하니까 납작해진 이미지가 나오더라고요. 그래서 조절 한 건데 정확한 비율을 몰라서 제가 화면에서 눈으로 보며대략적으로 정했습니다! 정확한 비율이 몇 인지 알면 수정해도 돼요!
frontend/src/hooks/useGetmember.ts
Outdated
const { data } = useQuery({ | ||
queryKey: [QUERY_KEYS.getMember], | ||
queryFn: getMember, | ||
staleTime: 30000, |
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.
💭 질문 💭
staleTime을 30초로 잡은 이유가 궁금합니다! 30초면 한 게임도 아니고, 한 라운드를 고려한 것도 아니라는 생각이 들어서 어떤 기준인지 궁금해요!
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.
제가 생각하기에 게임이 가장 빠르게 끝났을 때를 기준으로 잡았습니다! 임시로 정한 값이라 논의 후 수정해도 좋을 것 같아요
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.
서비스 정책상 변경이 없는 방에서 2시간 방이 유지되는 것으로 서버와 얘기가 되었습니다~! 그래서 그 시간에 맞춰 staleTIme도 2시간으로 정하면 좋을 것 같아요!!
const goToNicknamePage = () => { | ||
navigate(ROUTES.nickname); | ||
}; | ||
|
||
const handleRoomCreate = () => { |
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.
🙏 제안 🙏
줄바꿈 부탁드려요~!
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.
수정했습니다!
const handleRoomCreate = () => { | ||
goToNicknamePage(); | ||
setMemberInfo((memberInfo) => ({ ...memberInfo, isMaster: true })); | ||
navigate(ROUTES.nickname); |
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.
🌸 칭찬 🌸
와우 방생성 로직이 많이 간단해졌네요!!!
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.
💭 질문 💭
원래 이 함수로 닉네임페이지에서 방장과 초대받은 유저를 구분했는데, 쿠키로 그게 가능해진건가요??
홈에서 방 만들기 클릭 후 닉네임 페이지에서 새로고침 후 확인 눌러도 방 생성으로 잘 동작하는 건가요??
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.
방을 만들기 전에는 URL에 roomUuid
의 유무를 기준으로 방장 여부를 판단하고 있습니다!
const { roomUuid } = useParams(); | ||
|
||
// roomUuId가 없다 -> 초대링크를 받지 않은 master이다. | ||
const isMaster = !roomUuid; |
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.
💭 질문 💭
파라미터로 방장을 판단해도 괜찮을까요?? 사용자가 의도적으로 아무런 값만 추가해도 문제가 안생기나요? 엇 joinable API로 에러 페이지로 가니까 문제가 없을 수도 있겠네용
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.
유저가 URL
을 조작하지 않고 정상적으로 서비스를 실행했다면 괜찮을 것 같습니다. URL
을 변조한 경우에도 joinable API가 있기 때문에 에러 감지가 될 것으로 보입니다
const initializeState = (snap: MutableSnapshot) => { | ||
snap.set(memberInfoState, { memberId: 1, nickname: 'Test User', isMaster }); | ||
}; | ||
const customRenderWithMaster = (Component: React.ReactNode) => { |
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.
🙏 제안 🙏
아하 응답값을 바꾸기 위해 이렇게 작성했군요! 그런데 제 생각엔 util 함수에 server.use를 사용하는 건 애매한 것 같아요! 테스트 코드에서 server.use를 사용하는 게 유지보수 관점에서 적절하다고 생각합니다. 어떻게 생각하시나용
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.
server.use에서 방장
여부를 변경하는 함수가 여기저기 사용되어서 따로 함수로 만들긴 해야 할 것 같은데 그럼 테스트 코드용 공통 함수를 만드는 걸 생각하신건가요?
server.use(
http.get(MOCK_API_URL.getMember, async () => {
return HttpResponse.json(ROOM_AND_MASTER, { status: 200 });
}),
);
const navigate = useNavigate(); | ||
const { roomId } = useParams(); | ||
|
||
const exitRoomMutation = useMutation<void, Error, { roomId: number; memberId: number }>({ | ||
mutationFn: ({ roomId, memberId }) => exitRoom(roomId, memberId), | ||
onSuccess: () => { | ||
onSettled: () => { |
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.
💭 질문 💭
오호 성공하든 실패하든 쿠키지우고 홈으로 보내기 위해 onSettled를 사용한 것 같은데, 제가 이해한 게 맞을까용?
방나가기할 때 이렇게 처리하도록 바꾼건 좋은 것 같아요!
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.
네, 맞아요! 성공, 실패 상관없이 홈으로 보내는 게 맞다고 생각했어요!
Issue Number
#348
As-Is
기존에는 유저 정보를 recoil에서 전역상태로 관리하고 있었기 때문에 새로 고침 시 유저 정보가 사라져 게임이 진행이 되지 않는 상황 발생.
To-Be
roomUuid
값의 존재 유무를 통해 처리하는 방식으로 변경방장
으로 세팅해 주는 함수를recoil
에서API mocking
방식으로 변경Check List
Test Screenshot
(Optional) Additional Description
recoil 상태들이 사라지면서 여기저기 변경사항이 생겼습니다. 특히 테스트 쪽 유틸 함수들로 recoil을 없앴기 때문에 변경사항이 발생했습니다.
RoomSetting 컴포넌트 텍스트 위아래도 맞춤 되었습니다.
🌸 Storybook 배포 주소
🌸 Storybook 배포 주소