-
Notifications
You must be signed in to change notification settings - Fork 39
[이태경] Sprint9 #239
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
[이태경] Sprint9 #239
The head ref may contain hidden characters: "Next-\uC774\uD0DC\uACBD-sprint9"
Conversation
| try { | ||
| loadingRef.current = true; | ||
| await createTodoItem(todoText.trim()); | ||
| router.refresh(); // 서버컴포넌트 새로고침 |
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.
현재 컴포넌트가 아래와 같은 형태로 되어 있습니다.
page # 여기서 getTodoList()로 투두리스트 데이터 페칭
ㄴ TodoAddForm.tsx # 투두폼
ㄴ TodoListArea.tsx # 투두리스트 - 응답값 props로 전달
투두폼에서 submit시, TodoListArea에도 업데이트가 필요한데, 상태관리를 page에서 하고 있는게 아니기 때문에 router.refresh()를 적용하였는데,,, 현업에서도 쓰이는건지 궁금합니다.
클라이언트 상태는 유지하고 서버 컴포넌트만 새로고침 되는거라 page에서 다시 데이터페칭을 해서 TodoListArea로 내려주기 때문에 업데이트가 잘 되는거 같더라구요.
page에서 상태 관리를 하지 않는 이유는, 내부적으로 인풋 입력이나 체크박스 인풋이 바뀔때마다 불필요한 리렌더링이 발생하기 때문에 page에서는 상태관리를 하지 않았습니다.
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.
네, 잘 고민해보셨네요 :)
물론 현재 방식은 불필요한 리렌더링을 방지하기 위해서 고려된 결과지만, 구조를 봤을때 UX를 고려한다면 router.refresh()를 사용할때 전체 서버 컴포넌트를 새로 로드하게되므로 화면 깜빡임이 있을 수 있고, 만약 유저가 체크 박스 상태를 변경하고 바로 새 할일을 추가할때 낙관적 업데이트가 무효화되는 문제도 생길 수 있을 것 같아요.
따라서, page에서 상태관리를 하지 않는 방식은 유지하되, 할일을 추가할때
- React Query를 사용해 즉시 캐시 무효화 & 재검증을 처리하거나
router.refresh()로 전체 페이지를 새로고침하는대신 revalidatePath()을 사용해 캐시를 무효화하는 전략을 택하시는게 더 좋은 선택인것으로 보여지네요 :)
지금과 같은 경우 revalidatePath()는 서버 액션과 결합해서 쓰셔야하는데, 전체적인 과정은 공식 문서 링크 참고해보세요!
| setTodoAll((prevTodoAll) => | ||
| prevTodoAll.map((todo) => | ||
| todo.id === id ? { ...todo, isCompleted: !todo.isCompleted } : todo | ||
| ) | ||
| ); |
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.
useOptimistic 훅을 통해 낙관적 업데이트를 적용해봤습니다.
초기값으로 page에서 넘겨 받은 data 프롭스를 todoAll 상태에 적용시켜주고
todoAll을 useOptimistic의 초기값으로 적용하는 방법으로 진행하였습니다.
구글링을 했을 땐, api 실패를 했을 때에만 useOptimistic의 상태값이 이전 값으로 돌아간다고 해서, 처음에는 아래의 코드 순서로 진행을 하였습니다.
await updateTodoItem(id, updateData);
setTodoAll((prevTodoAll) =>
prevTodoAll.map((todo) =>
todo.id === id ? { ...todo, isCompleted: !todo.isCompleted } : todo
)
);
그런데, 성공을 했는데도 아주 잠깐 0.5초정도 이전 상태로 렌더링 되었다가 다시 원래대로 돌아오는 깜빡 거리는 현상이 있었습니다.
콘솔을 찍어서 테스트를 해보니, useOptimistic의 상태값이 이전 값으로 돌아갔다가 다시 돌아오더라구요.
그래서 api 요청 전에 기존 값을 변수에 저장하고, api 요청전에 setTodoAll을 먼저 진행하고, api 요청에 실패를 했을 때 다시 이전값으로 돌아갈 수 있도록 코드를 짜서 깜빡거림은 해결을 했습니다.
근데 이렇게 하니깐 useOptimistic 을 활용한 낙관적 업데이트가 아니라 그냥 useState로 하드코딩을 한 낙관적 업데이트가 된거 같아서 이상한 코드가 된거 같습니다..
혹시 코드상 잘못된게 있을까요?
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.
우선 toggleOptimisticState(id)를 호출한 후에 setTodoAll을 호출하면, useOptimistic의 초기값이 변경되어 깜빡임이 발생할거예요. 그리고 useOptimistic은 자체적으로 상태를 관리하는데, 지금 보면 별도로 useState로 todoAll을 관리하고 있어서 상태 업데이트 과정에서 동기화 문제가 생기기 쉬워요.
todoAll과 같은 별도 상태를 따로 사용하지않고 상태 업데이트를 위해 useOptimistic만 사용하고, useOptimistic의 초기값을 항상 data prop으로 고정하는 방식으로 바뀌면 깜빡임이 해결될거예요.
예시를 들어드릴게요!
const [optimisticState, toggleOptimisticState] = useOptimistic<
TodoItemType[],
number
>(data, (currentState, id) => {
return currentState.map((todo) =>
todo.id === id ? { ...todo, isCompleted: !todo.isCompleted } : todo
);
});| interface TodoResponseBase { | ||
| id: number; | ||
| tenantId: string; | ||
| name: string; | ||
| memo: string | null; | ||
| imageUrl: string | null; | ||
| isCompleted: boolean; | ||
| } | ||
|
|
||
| export interface PostTodoResponse extends TodoResponseBase {} | ||
|
|
||
| export interface PatchTodoResponse extends TodoResponseBase {} |
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.
스웨거에서 응답 스키마를 참고해서 타입을 지정했습니다.
근데 보니깐 POST일때나 PATCH일때 둘 다 똑같이 응답하더라구요.
그렇다고 타입 이름을 동일하게 가져가는건 아닌거 같아서 확장하는 식으로 이름만 바꿔서 적용했는데 그냥 하나의 타입명으로 정해서 내보내주는게 더 좋을까요?
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.
네, 불필요한 관리 포인트가 추가되니 하나의 타입명으로 내보내주시는게 좋을 것 같네요 :)
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.
태경님 고민을 깊게 하시는게 보이네요!
계속 고민하시고, 설계적으로 어떤게 나을지 학습하시다보면 좋은 결과물 만들수있을거예요 :)
잘하셨습니다! ㅎㅎ
주요 리뷰 포인트
- 최대 컨텐츠 사이즈에 따른 srcSet 최적화
- route groups, private folder 활용 방법 피드백
- 캐시 무효화 및 재검증과 관련한 피드백
- 상태 업데이트 및 동기화 문제와 관련한 피드백
| const Header = () => { | ||
| return ( | ||
| <header className="border-b border-slate200 bg-white"> | ||
| <div className="max-w-[1200px] mx-auto"> |
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.
최대 컨텐츠 사이즈가 1200px이라면, next/Image를 사용할때 이 최대 컨텐츠 너비에 맞춘 srcSet을 설정하면 어떨까요? 아래 아티클 참고해서 적용해보세요! :)
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.
LoadingSpinner, LoadingArea가 연관되어있기도하고 간단한 컴포넌트라 파일이 분리될 필요는 없어보여요.
하나의 파일에 Loading 관련 컴포넌트를 모아놓고 관리해보는건 어떨까요?
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.
라우팅에 관련없지만 비슷한 역할을 하는 파일끼리 그룹화하고싶었던거면 route group이나 private folder를 써보는건 어떨까요?
공식문서 Project Structure 섹션에 적용 방법이 잘 나와있답니다 :)
https://nextjs.org/docs/app/getting-started/project-structure#route-groups-and-private-folders
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.
해당 컴포넌트는 투두리스트의 컴포넌트에서만 쓰일 거 같아서 컴포넌트로 분리를 했었습니다 ㅎㅎ
근데 route group이나 private folder는 app 폴더 내에서 사용 하는 거로 알고 있는데,
컴포넌트에도 적용하기도 하는건가요?
app 폴더 내로 옮겨서 _component와 같은 폴더에 Empty 관련 컴포넌트를 구성해도 가져와서 사용을 하기도 하는지 궁금합니다.
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.
app 폴더 내로 옮겨서 _component private folder를 만드는 경우가 일반적입니다 :)
| try { | ||
| loadingRef.current = true; | ||
| await createTodoItem(todoText.trim()); | ||
| router.refresh(); // 서버컴포넌트 새로고침 |
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.
네, 잘 고민해보셨네요 :)
물론 현재 방식은 불필요한 리렌더링을 방지하기 위해서 고려된 결과지만, 구조를 봤을때 UX를 고려한다면 router.refresh()를 사용할때 전체 서버 컴포넌트를 새로 로드하게되므로 화면 깜빡임이 있을 수 있고, 만약 유저가 체크 박스 상태를 변경하고 바로 새 할일을 추가할때 낙관적 업데이트가 무효화되는 문제도 생길 수 있을 것 같아요.
따라서, page에서 상태관리를 하지 않는 방식은 유지하되, 할일을 추가할때
- React Query를 사용해 즉시 캐시 무효화 & 재검증을 처리하거나
router.refresh()로 전체 페이지를 새로고침하는대신 revalidatePath()을 사용해 캐시를 무효화하는 전략을 택하시는게 더 좋은 선택인것으로 보여지네요 :)
지금과 같은 경우 revalidatePath()는 서버 액션과 결합해서 쓰셔야하는데, 전체적인 과정은 공식 문서 링크 참고해보세요!
| setTodoAll((prevTodoAll) => | ||
| prevTodoAll.map((todo) => | ||
| todo.id === id ? { ...todo, isCompleted: !todo.isCompleted } : todo | ||
| ) | ||
| ); |
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.
우선 toggleOptimisticState(id)를 호출한 후에 setTodoAll을 호출하면, useOptimistic의 초기값이 변경되어 깜빡임이 발생할거예요. 그리고 useOptimistic은 자체적으로 상태를 관리하는데, 지금 보면 별도로 useState로 todoAll을 관리하고 있어서 상태 업데이트 과정에서 동기화 문제가 생기기 쉬워요.
todoAll과 같은 별도 상태를 따로 사용하지않고 상태 업데이트를 위해 useOptimistic만 사용하고, useOptimistic의 초기값을 항상 data prop으로 고정하는 방식으로 바뀌면 깜빡임이 해결될거예요.
예시를 들어드릴게요!
const [optimisticState, toggleOptimisticState] = useOptimistic<
TodoItemType[],
number
>(data, (currentState, id) => {
return currentState.map((todo) =>
todo.id === id ? { ...todo, isCompleted: !todo.isCompleted } : todo
);
});| interface TodoResponseBase { | ||
| id: number; | ||
| tenantId: string; | ||
| name: string; | ||
| memo: string | null; | ||
| imageUrl: string | null; | ||
| isCompleted: boolean; | ||
| } | ||
|
|
||
| export interface PostTodoResponse extends TodoResponseBase {} | ||
|
|
||
| export interface PatchTodoResponse extends TodoResponseBase {} |
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.
네, 불필요한 관리 포인트가 추가되니 하나의 타입명으로 내보내주시는게 좋을 것 같네요 :)
질문에 대한 답변
네, 서버 부하를 줄이는것또한 중요하죠 :) 캐싱 & 서버 컴포넌트로 클라이언트측 번들 크기를 줄이는 과정 또한 적절히 프로젝트/ 페이지 컨텐츠 특성에 따라 맞춰 사용하시면 좋고, |
요구사항
기본
주요 변경사항
스크린샷
멘토에게