-
Notifications
You must be signed in to change notification settings - Fork 5
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
커스텀 훅을 server data와 client data로 나눈다. #726
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.
리액트 쿼리 리팩토링 방향은 괜찮은 것 같습니다
그렇지만, 함수이름등을 수정해야할 것 같습니다
@@ -3,7 +3,7 @@ import React from 'react'; | |||
import { QueryClient, QueryClientProvider } from 'react-query'; | |||
import { BrowserRouter } from 'react-router-dom'; | |||
|
|||
import useGetCategoryArticles from '@/hooks/article/useGetCategoryArticles'; | |||
import useGetCategoryArticles from '@/hooks/queries/article/useGetCategoryArticles'; |
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.
queires 뭔가 이름이 마음에 안 들지만 어쩔 수 없네요... 더 좋은 이름 생각나면 들고오겠습니다
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.
대부분 queries를 많이 사용하더라고요..👻
|
||
import useInterval from '@/hooks/common/useInterval'; | ||
|
||
const useAutoSaveTempArticle = (handleTempSavedButtonClick: () => void) => { |
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.
해당 커스텀 훅은 common에 있을 필요가 없을 것 같습니다 tempArticle에 종속되어 있는 쿼리이기 때문에 해당 폴더 밑에 만드는 것이 더 좋을 것 같습니다
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.
음 common보다는 tempArticle아래에 있는것이 좋을것 같습니다. queries와 별개로 이러한 로직들을 처리하는 훅에 대해서의 폴더이름은 어떤것이 좋을까요? bussiness
, service
, logic
등등 괜찮은거 있으신가요?
import useInterval from '@/hooks/common/useInterval'; | ||
|
||
const useAutoSaveTempArticle = (handleTempSavedButtonClick: () => void) => { | ||
useInterval(handleTempSavedButtonClick, 120000); |
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.
120000을 상수로 뺴는게 어떨까요?
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 preventRefresh = (e: BeforeUnloadEvent) => { |
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.
음.... preventRefreshEvent 어떤가요? 함수이름
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.
이벤트 타입 지정 해준 건 좋네요 👍
import { postImageUrlConverter } from '@/api/image'; | ||
import { Editor } from '@toast-ui/react-editor'; | ||
|
||
const useConvertImageUrl = (content: MutableRefObject<Editor | 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.
converter도 좋긴하지만, 길이를 줄여준다는 함수 이름에 들어가면 좋을 것같습니다
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.
길이를 줄이다는 Shorten이라는 영어를 이용하네요! useShortenImageUrl로 변경하였습니다.
const { category } = useParams(); | ||
const [isAnonymous, setIsAnonymous] = useState<boolean>(false); | ||
const [tempArticleId, setTempArticleId] = useState<'' | number>(tempId); | ||
const [initContent, setInitContent] = useState<string>(''); | ||
const [title, setTitle] = useState(''); | ||
const [categoryOption, setCategoryOption] = useState<string>(category ? category : ''); | ||
const [isValidTitleInput, setIsValidTitleInput] = useState(true); | ||
const [hashTags, setHashTags] = useState<string[]>([]); | ||
|
||
const titleInputRef = useRef<HTMLInputElement>(null); | ||
const content = useRef<Editor | null>(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.
이 state들은 page component에서 관리할 예정인가요?
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.
넵 일단은 client state들은 page Component에 두고 분리할 필요성을 가진다면 hook으로 빼면 좋을것 같습니다.
close #725
문제상황
react-query의 목적인 server state와 client state의 분리를 무시하고 코드를 작성하고 있었다. react-query의 커스텀 훅 네이밍을
useGet...
,usePost...
으로 가져갔으나 그안에 react의 ui를 관리하는 client state까지 같이 섞여있는 훅이 되버렸다.따라서 client state와 server state각각의 관심사의 분리가 필요했다.
어떻게 문제를 해결하였나요??
기존에 존재하는
useGet...
,usePost...
등의 react query 커스텀 훅을 분리하는 기준을 재정립하였습니다.server state를 관리하는 훅은 오직 useQuery와 useMutation 그리고 이것이
성공
실패
로딩
중일때만을 알고 있으면 됩니다.성공할때 client의 상태를 바꾸고 싶다면 client state를 바꾸는 함수를
react query 커스텀 훅
으로 인자로 전달하여 이용합니다.useMutation훅을 이용할때는
mutate를 하는 함수
혹은mutate 그자체
를 커스텀 훅의 반환값으로 전달하여 client 단에서 호출할 수 있도록 설정하였습니다.중심 리뷰 사항
현재는 테스트 상으로 WrtingArticles에 있는 로직들만 분리해보았습니다. 이 방식이 괜찮으면 그대로 전체 리팩토링 진행하겠습니다.
또한 react query에 사용되는 훅은 hooks 내의 quries라는 훅으로 분리하였습니다. client state의 모든것을 훅으로 빼는것은 불 필요하다고 생각되어서 너무 비대해진 로직이나 중복되는 로직만 분리하는것으로 하였습니다.