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.

Sign up for GitHub

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

Jump to bottom

feat: add token setting and store setting #39

Merged
merged 5 commits into from
Dec 14, 2024
Merged

feat: add token setting and store setting #39

merged 5 commits into from
Dec 14, 2024

Conversation

Jeong-jj
Copy link
Member

@Jeong-jj Jeong-jj commented Dec 13, 2024 •
edited
Loading

무엇을 위한 PR인가요?

  • 신규 기능 추가 : api interceptor
  • 버그 수정 :
  • 리펙토링 : store
  • 문서 업데이트 :
  • 기타 :

변경사항 및 이유

  • api setting에 token interceptor 추가
  • store setting 추가
    • 기존 localStorage에 저장하던 userId 값을 store에 저장하도록 수정
    • setting 설정 페이지에서 임의 userId로 넣고 있던 param값을 로그인시 저장된 값을 가져와 호출하도록 수정

PR 특이 사항

  • zustand 설치
  • 추후 api 수정이 이루어져 token이 들어오면 interceptor 쪽 변수값 수정 필요
    • 현재는 token이 리턴되지 않아 빈 스트링으로 설정
  • 리뷰 이후 memberId를 userId로 네이밍 일치화 및 type을 number로 적용
    • 백엔드에서 post메소드의 key값을 memberId로 정해둔 경우를 제외하고 적용

Issue Number

#15

@Jeong-jj Jeong-jj self-assigned this Dec 13, 2024
confidential-nt
confidential-nt approved these changes Dec 14, 2024
View reviewed changes
src/pages/answer-create/index.tsx Outdated
const userInfo = res.data.data;
setUserNicname(userInfo.nickname);

answerAPI.create({
memberId: Number(storedId),
memberId: Number(userId),
Copy link
Contributor

Choose a reason for hiding this comment

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

user ID 와 member ID... 통일 했으면 좋겠는데 다른 분들 의견은 어떤가요?

Copy link
Member Author

Choose a reason for hiding this comment

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

의견 주시는 방향으로 수정할게요!

Copy link
Contributor

Choose a reason for hiding this comment

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

userId가 좋을 것 같네요! 여기는 number타입으로 받고있군요 😢

Copy link
Member

Choose a reason for hiding this comment

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

저는 userId와 memberId는 명확하게 다른거라고 생각하는데, 지금 저희가 쓰는 느낌으로는 ㅡmemberId보단 userId가 더 적합해보이기는하네요

Copy link
Member Author

Choose a reason for hiding this comment

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

넵넵! 그런데 혹시 다른 느낌이라면 어떤 차이로 느끼고 계실까요? 궁금증입니당

Copy link
Member

Choose a reason for hiding this comment

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

member는 어떤 그룹에 속한 느낌이고 user는 유저의 계정 그 자체를 표현하는거라고 생각합니다!
유저와 유저 아이디는 1:1이고, 유저와 멤버아이디는 1:n인거죠!

hyjoong
hyjoong approved these changes Dec 14, 2024
View reviewed changes
Copy link
Contributor

@hyjoong hyjoong left a comment •
edited
Loading

Choose a reason for hiding this comment

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

고생하셨습니다!
현재 여러 곳에서 String(userId)로 타입을 변환하는 패턴이 반복되고있네요.
store에서 userId를 저장할 때부터 String타입으로 관리하면 좋을 것 같습니다!

@Jeong-jj
Copy link
Member Author

Jeong-jj commented Dec 14, 2024 •
edited
Loading

고생하셨습니다! 현재 여러 곳에서 String(userId)로 타입을 변환하는 패턴이 반복되고있네요. store에서 userId를 저장할 때부터 String타입으로 관리하면 좋을 것 같습니다!

맞습니다!ㅠㅠ 그런데 api마다 같은 userId이지만 타입이 number와 string으로 다르게 설정된 경우가 있습니다.
그래서 오히려 어떤 곳에선 String()변환 필요 없이 바로 넣어도 문제가 되지 않는 경우가 있어 store에서 작업을 하지 않았습니다.

api type을 일치시키면 문제가 해결될 것 같아요, id의 경우 number 유지가 맞다고 생각되는데 괜찮다면 number로 일치시켜도 괜찮을까요
@confidential-nt @hyjoong @s2oy

@hyjoong
Copy link
Contributor

hyjoong commented Dec 14, 2024

네 좋습니다!

@Jeong-jj
refactor: unify userId key naming rules
b58f92c 
apply the rules except those set to key values in the post method param
@Jeong-jj
Copy link
Member Author

리뷰 사항 반영하였습니다.

  • memberId를 userId로 네이밍 일치화 및 type을 number로 적용
    • 백엔드에서 post메소드의 key값을 memberId로 정해둔 경우를 제외하고 적용하였습니다.

@Jeong-jj Jeong-jj merged commit e138052 into main Dec 14, 2024
@Jeong-jj Jeong-jj mentioned this pull request Dec 14, 2024
Merged
5 tasks
@Jeong-jj Jeong-jj deleted the feat/settings branch December 18, 2024 07:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@s2oy s2oy s2oy approved these changes

@confidential-nt confidential-nt confidential-nt approved these changes

@hyjoong hyjoong hyjoong approved these changes

Assignees

@Jeong-jj Jeong-jj

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Jeong-jj @hyjoong @confidential-nt @s2oy