-
Notifications
You must be signed in to change notification settings - Fork 3
fetch Instance 및 fetch 메서드 코드 구현 #95
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
Conversation
Zero-1016
left a comment
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.
규회님은 주석이나 문서화를 되게 꼼꼼하게 하시는군요 보기 좋습니다. 직접 fetchInstance를 구현하시고 멋지십니다!
궁금한 사항이나, 소소한 TMI 위주로 리뷰를 남겨드려요!
별도로 질문이 있습니다! 배럴 파일을 되게 많이 사용하시는데 경로가 짧아지는 거 이외에 장점이 있나요? 어떤 이점으로 사용하는지 궁금합니다. (ex: fsd 구조이다 보니 폴더 구조가 복잡해서 배럴 파일로 최상단에서 사용)
| // withToken이 true일 때만 토큰을 헤더에 추가 | ||
| if (withToken) { | ||
| const token = | ||
| typeof window !== 'undefined' ? localStorage.getItem('token') : null; |
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.
withToken 일 때 token이 없다면 의도치 않은 요청이 아닐까요? 에러가 나타나면 좋을 듯합니다.
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을 넣어야할 api와 넣지 말아야할 api를 구분지어서 request를 보내기 위해서 짠 코드긴 했습니다.
저희가 메인 페이지 내에서 주식 데이터를 불러올때 로그인없이도 받아올 수 있다보니 그랬던 것 같습니다 (:
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.
위에서 봤을 때는 withToken을 명시해야 되는 거 같아서 없다면 에러가 발생해야 하지 않을까라는 생각이 있긴 했네요
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.
저도 비슷한 생각입니다!
저는 단순하게 생각하여 token이 없다면 에러처리를 해야할것같은데, token을 넣지 말아야할 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.
@KimKyuHoi 그리고 리뷰가 아닌 단순한 궁금증인데, token관련한 코드가 3번 정도 나타나는데 보통 그런식으로 여러번 확인하고 할당하는걸까요?
처음 변수 선언때도 localstorage에서 받아오고 아래 withtoken에 의해 또한번 받아오면서, header를 선언하는 부분도 token의 여부에 따라 달라지는 것이 두번 반복되는것으로 보여서요!
제가 instance는 해본적이 없어서 다 다른 경우에 대응하기 위한 부분일까요?
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이 없다면 에러처리를 해야할것같은데, token을 넣지 말아야할 api는 어떤것이 있을지 궁급합니다!
초기 로그인같은 경우는 토큰없이 쓰기때문에 넣긴 했습니다.
그리고 리뷰가 아닌 단순한 궁금증인데, token관련한 코드가 3번 정도 나타나는데 보통 그런식으로 여러번 확인하고 할당하는걸까요?
처음 변수 선언때도 localstorage에서 받아오고 아래 withtoken에 의해 또한번 받아오면서, header를 선언하는 부분도 token의 여부에 따라 달라지는 것이 두번 반복되는것으로 보여서요!
제가 instance는 해본적이 없어서 다 다른 경우에 대응하기 위한 부분일까요?
그리고 첫번째 토큰 로직은 저희가 스토리지 내에서 토큰을 가지고 오기로 결정했으므로 스토리지에서 토큰을 받아오는 로직입니다.
두번쨰 로직은 저희가 토큰을 쓸껀지 안 쓸껀지 확인하는 로직이구요. default는 true이나 false설정을 걸 경우 토큰을 안 쓸 수 있습니다.
세번째 로직은 가지고 온 토큰을 header내에 Authorization Bearer에 넣는 로직입니다.
제가 로직 관련해서 주석처리를 해놓긴 했는데 나중에 궁금해시면 주석처리한 부분 따라 읽으면서 코드 내려가시면 읽기 편하실껍니다.
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.
@handje 객체명 교체하였습니다~ withToken -> includedAuthToken으로 교체했습니다~
src/frontend/apps/web/src/shared/services/apis/fetch-method.api.ts
Outdated
Show resolved
Hide resolved
src/frontend/apps/web/src/shared/services/models/error/authorization-errors.ts
Outdated
Show resolved
Hide resolved
오호 배럴파일은 쓰는이유는 import문이 간단해지기 때문입니다. fsd를 사용하게 되면 결국엔 3 depth까지 넘어가기 때문에 절대경로를 쓰게 된다면 엄청난 길이의 Import문을 보실 수 있는데 그걸 방지하기 위해 배럴 파일을 넣긴합니다. |
|
@Zero-1016 @KimKyuHoi 지형님과 규회님의 엄청난 코드와 리뷰덕에 많이 배우고있습니다!! 멋져용 |
handje
left a comment
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.
좋습니다!
Pull request
Related issue
Motivation and context
Solution
prettier printWidth 가독성이 너무 구려 180 -> 80으로 변경했습니다.
파일 분리 apis 폴더를 services 폴더명은 변경하였습니다.
. ├── apis │ ├── fetch-instance.api.ts │ ├── fetch-method.api.ts │ └── index.ts ├── index.ts └── models ├── error │ ├── authorization-errors.ts │ ├── index.ts │ └── workspace-errors.ts ├── index.ts └── types ├── fetch-types.d.ts └── index.tsHow has this been tested
Types of changes
Checklist