-
Notifications
You must be signed in to change notification settings - Fork 2
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
[Refactor] token 필요없는 fetch custom hook 만들기 #445
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.
고생하셨습니다 !
하나의 의견이 있는데, 현재 headers
에서 Record
유틸리티 타입으로 string, string
key-value를 받고 있는데, 잘못된 key 값을 헤더에 넣었을 시 에러가 발생할 수도 있을 것 같다는 생각을 했습니다.
fetch
api를 사용하기로 했으므로 Union
타입으로 headers
key 값에 들어갈 스트링을 제한해주면 어떨까 싶습니다 !
|
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.
좋아요 ~ ~ ~ ~ 🚀 🚀
저도 주용오빠와 같은 맥락의 의견 내자면 method option도 타입 좁혀주는거 어떠신가요 ~??
물론 header key 에 비해서는 저희가 잘 알고있는 친구들이라 추론 없이도 큰 탈 없이 사용해줄 수 있겠지만
기존 axios 사용시에는 AxiosInstance 가 가지는 메소드로 추론이 됐었는데 현재 쓰이는 RequestInit의 method는 string으로만 되어있어서
대표적으로 사용하는 'GET' | 'POST' | 'PUT' | 'PATCH' | 'DELETE' 타입만 아주 간단히 정의해주면 axios쓸때랑 똑같은 기능을 얻을 수 있어서 추가해줘도 좋을 것 같아용
완동 입니다 ~ 🚀 |
Related Issue : Closes #444
🧑🎤 Summary
🧑🎤 Comment
코드 짜기 전에는 hook으로 분리해줄까 했었는데
짜고 보니 분리할 필요가 없더라고요?
원래 방식처럼 그냥 바로 instance 가져다가 사용하면 됐어요
사용법은 비슷해요
첫 번째 인자로 url 넣어주고 두 번째 인자로 options들 넣어주면 됩니다~
단 body는 JSON.stringify()로 감싸줘야 해요!