-
Notifications
You must be signed in to change notification settings - Fork 39
[이태경] Sprint10 #249
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
[이태경] Sprint10 #249
The head ref may contain hidden characters: "Next-\uC774\uD0DC\uACBD-sprint10"
Conversation
src/hooks/useUpdateCheck.ts
Outdated
| createToast({ message: "다시 시도해주세요." }); | ||
| }, | ||
| onSettled: () => { | ||
| if (!(queryClient.isMutating() === 1)) return; |
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.
투두리스트에서 체크아이템을 연속적으로 클릭하여 여러개의 체크아이템 상태가 변화될 때 api가 성공하면 무효화 되도록 설정을 했었습니다.
그런데 불규칙적으로 무효화가 이전의 데이터로 덮어씌어져서 제대로 적용이 안될 때가 있더라구요!
분명히 체크를 해서 done 상태가 되었는데, 여전히 todo 상태에 남아있다던가 등등....
그래서 동시성을 제어하고 일관성있게 데이터를 유지하는 방법으로 isMutating()을 이용했는데, 혹시 더 좋은 방법이 있을까요?
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.
지금 구현 방식을 보니, isMutating() === 1 조건으로 마지막 mutation이 완료될 때만 무효화하도록 되어 있네요! 이 과정에서 isMutating()은 전체 앱의 mutation 상태를 확인하도록 설계되어있어서 다른 mutation 과정과 같이 평가될거예요. 따라서, 여러개의 mutation이 동시에 실행되는 상황이라면 타이밍 이슈가 충분히 발생할 수 있습니다 :)
더 안정적인 방식으로 바꿔보려면, mutation 과정마다 독립적인 실행이 보장되는 onMutate 콜백에서 각 mutation이 성공할 때마다 즉시 서버 상태와 동기화하고, cancelQueries를 사용해 진행 중인 쿼리를 취소해주는 과정이 필요할것같네요 :)
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.
만약 각 훅이 독립적이고 명확한 역할을 가지고있다면 디버깅의 용이함을 위해 분리하지 않는 방식이 유리합니다. 하지만, 여러 훅에 걸쳐 일관된 패턴이 보인다면 재사용성을 향상하기 위해 분리하셔도 좋습니다.
| try { | ||
| const { url } = await postImage(formData); | ||
| return url; | ||
| } catch (error) { | ||
| console.error(error); | ||
| createToast({ message: "이미지 업로드 실패. 다시 시도 해주세요." }); | ||
| throw error; | ||
| } finally { | ||
| setIsUploading(false); | ||
| } | ||
| }; |
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.
api 요청이 필요한 곳은 코드의 일관성을 위해서 리액트 쿼리를 사용하였는데,
이미지 업로드는 캐싱이 필요하지 않다고 생각해서 fetch로 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.
일관성을 지키려하기보다는, 적극적으로 페이지/콘텐츠에 맞는 data fetching/ 캐싱 전략을 취하도록 쿼리 키 기반 캐싱이 필요할 경우에만 RQ를 사용해주시는것도 성능을 고려한 좋은 선택이랍니다 :)
addiescode-sj
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.
수고하셨습니다!
워낙 꼼꼼히 고민하고 코드 작성하신게 보여서,
이번에도 기능보다는 설계 관련한 피드백을 드려봤어요 :)
주요 리뷰 포인트
- RQ 관련 코드 중복 제거 (wrapper 컴포넌트화)
- Toast 컴포넌트 리팩토링 제안
- 임의의 id값을 생성하지않고 onSuccess 콜백을 통해 서버에서 전달되는 id값으로 처리하기
- isMutating 활용하지않고 코드 안정성 높이기 (독립적 실행이 보장되는 onMutate 콜백 활용)
| const queryClient = getQueryClient(); | ||
|
|
||
| await queryClient.fetchQuery(todoQueries.detailOptions(itemId)); | ||
|
|
||
| return ( | ||
| <div className="max-w-[996px] mx-auto pt-6"> | ||
| <HydrationBoundary state={dehydrate(queryClient)}> | ||
| <TodoDetailArea itemId={itemId} /> | ||
| </HydrationBoundary> | ||
| </div> |
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.
다른 페이지에서도 React Query 관련 코드가 중복될 가능성이 있다면 컴포넌트화해서 중복을 제거해보는게 어떨까요~? children을 가진 wrapper 컴포넌트를 만들면 될것같네요 :)
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.
강사님 안녕하세요~
심화 프로젝트를 진행하면서 하이드레이션 바운더리를 컴포넌트화 하게 되었습니다.
그런데, 처음엔 dehydrate까지 공용 컴포넌트에 포함을 하려고 했는데
각 페이지마다 useInfiniteQuery를 사용할 수도 있고, useQuery를 사용할 수도 있어서 직렬화 하는 코드는 컴포넌트를 사용하는 사람에게 넘기는 식으로 아래와 같이 코드를 구성했습니다.
결론적으로 전체 페이지가 csr로 변경되면서 이 컴포넌트는 사용되지 않았지만,, 혹시 fetchInfiniteQuery 나 fetchQuery를 분리해서 사용해야 한다면, 좀 더 좋은 코드가 없을지 의견을 여쭙고 싶습니다 :)
import { DehydratedState, HydrationBoundary } from '@tanstack/react-query';
import { ReactNode } from 'react';
interface Props {
state: DehydratedState;
children: ReactNode;
}
export default function QueryHydration({ state, children }: Props) {
return <HydrationBoundary state={state}>{children}</HydrationBoundary>;
}
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.
지금도 이정도 규모의 프로젝트에서는 충분히 괜찮지만, 좀 더 나중을 위해 재사용성을 최대한 끌어올리는 방향으로 정해본다면 가장 범용적인 용도의 HydrationBoundary 래퍼를 하나 만들고, 각 용도에 따라 맞추어 설계된 HydrationBoundary 래퍼들을 따로 만들어 합성하여 쓰시는것도 방법입니다 (ex. HydrationWrapper.Prefetch).
그리고 이 과정에서 prefetch 동작에 관련한 유틸 함수들을 만들어 같이 사용하시는것또한 타입 안정성을 강화할수있는 좋은 방법이 될것같네요 :) @LeeTaegyung
| const [isClient, setIsClient] = useState(false); | ||
| const toasts = useToastStore((state) => state.toasts); | ||
|
|
||
| useEffect(() => { | ||
| setIsClient(true); | ||
| }, []); | ||
|
|
||
| if (!isClient) return; |
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.
해당 컴포넌트에서 몇가지 불필요한 처리가 있는것같아요.
우선, use client 디렉티브를 붙였기때문에 클라이언트에서 컴포넌트가 해석되고 렌더링될텐데 state update를 통해 클라이언트임을 식별하는 이유가 있을까요?
다른 구현 의도가 있는게 아니라면, 불필요한 대응으로 보여집니다 :)
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.
추가적으로, 일반적으로 토스트를 한꺼번에 여러개 띄우지 않을텐데 토스트를 배열로 관리할 필요가 없어보여요. 토스트를 배열로 관리하다보면 각 토스트가 띄워지는 시점에 대한 타이밍 조절이나 Id로 어떤 토스트를 먼저 띄울지 등등 관련 대응이 필수적이라, 마찬가지로 구현 의도에 필수적인 과정이 아니라면 한번에 하나의 토스트를 띄울수있게 코드를 간소화해보면 좋을것같네요 :)
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 showToast = useToastStore((state) => state.showToast);
showToast("메시지");
// 토스트 숨김 (필요시)
const hideToast = useToastStore((state) => state.hideToast);
hideToast();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.
해당 컴포넌트에서 몇가지 불필요한 처리가 있는것같아요.
우선, use client 디렉티브를 붙였기때문에 클라이언트에서 컴포넌트가 해석되고 렌더링될텐데 state update를 통해 클라이언트임을 식별하는 이유가 있을까요?
다른 구현 의도가 있는게 아니라면, 불필요한 대응으로 보여집니다 :)
이건 'document is not defined' 에러를 대응하기 위해 사용을 했습니다! 저도 처음엔 클라이언트 컴포넌트이기 때문에 여기에서 에러가 발생할거라 생각을 못했는데 아무래도 클라이언트 컴포넌트이긴 하지만, 서버에서도 한번더 실행이 되면서 document를 찾지 못해 발생한 문제인거 같았어요!
구글링을 해보니깐 좀 다양한 방법이 나왔었고 그 중 가장 편리하게 대응할 수 있는, typeof document === 'undefined' 조건문으로 분기 처리를 해줬는데 왜인지 모르겠지만.... 계속 오류가 발생하더라구요... 이건 도저히 이유를 모르겠어서 우선 좀 더 익숙한 useEffect를 사용해주었습니다.
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.
지금 토스트 포털을 사용하며 document.body를 사용하고있는데, 해당 컴포넌트가 서버에서 먼저 실행될때 이 document에 접근하지못해 발생하는 문제입니다 :) 동적 임포트를 통해 클라이언트 환경에서만 토스트 포털을 사용하도록 바꿔주면 해결될것같네요.
const ToastPortal = dynamic(() => import('./ToastPortal'), {
ssr: false,
loading: () => null,
});| const prevTodos = queryClient.getQueryData<TodoItemType[]>(["todos"]); | ||
|
|
||
| const newTodos: TodoItemType = { | ||
| id: 9999, // 임시 id 값 설정 |
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.
임시로 임의의 id값을 생성하는것보다는,
요청 성공 시 응답받는 todo 데이터에 id값이 있다면 해당 id를 사용해주는편이 좋을것같아요. onSuccess 콜백에서 해당 과정을 처리하는것으로 바꿔볼까요?
예시)
onSuccess: (data: TodoResponseType) => {
// 서버에서 생성된 실제 데이터로 캐시 업데이트
queryClient.setQueryData(QUERY_KEY_TODOLIST, (todos: TodoItemType[]) => {
const newTodo: TodoItemType = {
id: data.id,
name: data.name,
isCompleted: data.isCompleted,
};
return [newTodo, ...(todos || [])];
});
showToast("할 일 추가 성공!");
},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 메서드로 쿼리키를 초기화 해주고 있는데, 이 방법보다는 setQueryData로 수동적으로 추가해주는게 좀 더 좋을까요?
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.
낙관적 업데이트의 경우에는 동기화 문제가 생기기 쉬우니, 나중에 setQueryData로 추가하기보다는 onSettled에서 쿼리 초기화를 해주는게 더 안정적일것같습니다 :)
src/hooks/useUpdateCheck.ts
Outdated
| createToast({ message: "다시 시도해주세요." }); | ||
| }, | ||
| onSettled: () => { | ||
| if (!(queryClient.isMutating() === 1)) return; |
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.
지금 구현 방식을 보니, isMutating() === 1 조건으로 마지막 mutation이 완료될 때만 무효화하도록 되어 있네요! 이 과정에서 isMutating()은 전체 앱의 mutation 상태를 확인하도록 설계되어있어서 다른 mutation 과정과 같이 평가될거예요. 따라서, 여러개의 mutation이 동시에 실행되는 상황이라면 타이밍 이슈가 충분히 발생할 수 있습니다 :)
더 안정적인 방식으로 바꿔보려면, mutation 과정마다 독립적인 실행이 보장되는 onMutate 콜백에서 각 mutation이 성공할 때마다 즉시 서버 상태와 동기화하고, cancelQueries를 사용해 진행 중인 쿼리를 취소해주는 과정이 필요할것같네요 :)
| try { | ||
| const { url } = await postImage(formData); | ||
| return url; | ||
| } catch (error) { | ||
| console.error(error); | ||
| createToast({ message: "이미지 업로드 실패. 다시 시도 해주세요." }); | ||
| throw error; | ||
| } finally { | ||
| setIsUploading(false); | ||
| } | ||
| }; |
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.
일관성을 지키려하기보다는, 적극적으로 페이지/콘텐츠에 맞는 data fetching/ 캐싱 전략을 취하도록 쿼리 키 기반 캐싱이 필요할 경우에만 RQ를 사용해주시는것도 성능을 고려한 좋은 선택이랍니다 :)
질문에 대한 답변
두개의 메서드가 하는 일이 다르긴 해요. 따라서 잘 이해하신것처럼 두개를 동시에 사용하실 필요는 없고, 조건부 무효화 방식으로 일관성 있게 캐시를 갱신하시는게 나을것같네요 :) |
요구사항
기본
할 일 수정
할 일 삭제
주요 변경사항
스크린샷
수정

투두리스트 수정사항 적용된 모습

삭제한 모습

배포 사이트
https://delicate-bonbon-5f2801.netlify.app/
멘토에게
근데 setQueryData로 먼저 변경된 데이터로 캐시를 즉시 수정해 적용해주고 api가 완료되었을때 invalidateQueries로 무효화 시켜 주는 부분이 저는 좀 중복이 되는 것 같다는 생각이 좀 들었습니다. 두개의 메서드가 하는 일이 비슷한 느낌이어서 굳이 invalidateQueries를 사용해서 또 한번 더 리렌더링을 발생시킬 필요가 있나? 라는 생각이 들기도 했습니다.