Skip to content
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

feat: 피드 수정 구현 #1265

Merged
merged 13 commits into from
Jan 21, 2024
Merged

feat: 피드 수정 구현 #1265

merged 13 commits into from
Jan 21, 2024

Conversation

seojisoosoo
Copy link
Member

@seojisoosoo seojisoosoo commented Jan 9, 2024

🤫 쉿, 나한테만 말해줘요. 이슈넘버

🧐 어떤 것을 변경했어요~?

  • 피드 수정을 구현했어요.

🤔 그렇다면, 어떻게 구현했어요~?

  • 기존의 FeedUploadPage를 재활용하려고 했어요, 그래서 isEdit을 prop으로 내려받도록 했습니다.(isEdit true이면 PUT, 아니면 POST가 되도록이요!)
  • 또한 feed/edit/[id]로 라우팅해서 query.id를 prop으로 내려주었어요. 그 값을 이용해 const { data: postData } = useGetPostQuery(feedId); 로 데이터를 가져왔어요. 만일 postData가 없으면(1.실제로 값이 없거나 2. 수정이 아닌 업로드인 경우) null이나 빈값이 되도록 다음과 같이 initialData를 선언해주었어요.
  const initialData = {
    mainCategoryId: postData?.posts.categoryId ?? null,
    categoryId: postData?.posts.categoryId ?? null,
    title: postData?.posts.title ?? '',
    content: postData?.posts.content ?? '',
    isQuestion: postData?.posts.isQuestion ?? false,
    isBlindWriter: postData?.posts.isBlindWriter ?? false,
    images: postData?.posts.images ?? [],
  };

❤️‍🔥 당신이 생각하는 PR포인트, 내겐 매력포인트.

useUploadFeedData

const [feedData, setFeedData] = useState(initialForm);

  • 만일, 수정을 눌러서 수정페이지로 들어오면 feedData의 초기값이 initialForm으로 잘 들어오는데요! 새로고침을 하게 되면, feedData가 initialForm이 아닌 null, false, []로 초기화가 되더라고요.. (간혹 처음에도 initialForm으로 초기화가 안 될 때가 있습니다ㅜ)
  • props로 받는 initialForm(부모컴포넌트에서는 initialData)을 변수가 아닌 state로 변경해보아도 동일한 이슈가 생기더라구요, useEffect를 사용해서 setFeedData해주면 무한리렌더링이 발생해서 좋은 방법이 없을지 논의하고 싶습니다!

코드리뷰 반영하면서 리팩토링한 결과

🤔 그렇다면, 어떻게 구현했어요~?

  • 원래 FeedUploadPage 안에서 handleEditFeed, handleUpdateFeed를 불러왔는데, FeedUploadPage 바깥에서 불러오고 props으로 내려주었어요.
  • FeedUploadPage 컴포넌트에, initialForm, onSubmit, editingId을 prop으로 내려주었어요.
  • 수정의 경우, 데이터를 읽어온 뒤 data != null인 경우에만 FeedUploadPage컴포넌트가 보여지도록 해서, 새로고침을 해도 수정 데이터가 바로 잘 들어와요!!

📸 스크린샷, 없으면 이것 참,, 섭섭한데요?

스크린샷 2024-01-09 오후 7 21 58

@seojisoosoo seojisoosoo self-assigned this Jan 9, 2024
@github-actions github-actions bot requested a review from juno7803 January 9, 2024 10:22
Copy link

github-actions bot commented Jan 9, 2024

✨✨ 스토리북으로 확인하기 ✨✨

Copy link

github-actions bot commented Jan 9, 2024

🚀 프리뷰 배포 확인하기 🚀

https://b719f4a2.sopt-internal-dev.pages.dev

Comment on lines 30 to 44
export const useSaveEditFeedData = (feedId: string) => {
const router = useRouter();
const queryClient = useQueryClient();
const { logSubmitEvent } = useEventLogger();

return useMutation({
mutationFn: (reqeustBody: RequestBody) => editFeed.request(reqeustBody),
onSuccess: async () => {
logSubmitEvent('editCommunity');
queryClient.invalidateQueries({ queryKey: useGetPostsInfiniteQuery.getKey('') });
queryClient.invalidateQueries({ queryKey: getPost.cacheKey(feedId) });
await router.push(playgroundLink.feedList());
},
});
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p1; 여기는 API 호출부를 선언하는 곳인데, 이 훅은 비즈니스 로직의 너무 많은 부분을 알고 있는 것 같아요..! mutation이라 공유해야 할 키도 없으니 이 훅 없이 그냥 컴포넌트에서 바로 사용해도 될 것 같아요. 또는 이렇게 하되 success 후의 로직은 이렇게 mutate 함수에서 작성하면 좋을 것 같아요.

Suggested change
export const useSaveEditFeedData = (feedId: string) => {
const router = useRouter();
const queryClient = useQueryClient();
const { logSubmitEvent } = useEventLogger();
return useMutation({
mutationFn: (reqeustBody: RequestBody) => editFeed.request(reqeustBody),
onSuccess: async () => {
logSubmitEvent('editCommunity');
queryClient.invalidateQueries({ queryKey: useGetPostsInfiniteQuery.getKey('') });
queryClient.invalidateQueries({ queryKey: getPost.cacheKey(feedId) });
await router.push(playgroundLink.feedList());
},
});
};
export const useSaveEditFeedData = (feedId: string) => {
const router = useRouter();
const queryClient = useQueryClient();
const { logSubmitEvent } = useEventLogger();
return useMutation({
mutationFn: (reqeustBody: RequestBody) => editFeed.request(reqeustBody)
});
};
/// 사용부에서
const { mutate } = useSaveEditFeedData(id);
mutate({
onSuccess: async () => {
logSubmitEvent('editCommunity');
queryClient.invalidateQueries({ queryKey: useGetPostsInfiniteQuery.getKey('') });
queryClient.invalidateQueries({ queryKey: getPost.cacheKey(feedId) });
await router.push(playgroundLink.feedList());
}});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉 정말 동의합니다! mutate함수에다 onSuccess 로직을 작성해도 잘 되는 군요..!!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

더 나아가 쿼리와 다르게 key를 따로 관리해 줄 필요도 없어서 useSaveEditFeedData 를 따로 만들어두지 않고 useMutation 을 바로 써도 될 것 같네용..!

Comment on lines +23 to +29
const findMainCategory = (categoryId: number | null, mainCategoryId?: number | null) => {
const parentCategory =
(categoryData &&
categoryData.find(
(category) => category.id === mainCategoryId || category.children.some((tag) => tag.id === categoryId),
)) ??
null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p3; 무엇을 하는 함수인가요? 여기서의 Main Category 가 뭔지 알 수 없는 것 같아요.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

main category는 자유, 파트, SOPT 활동, 홍보, 취업/진로와 같은 부모 카테고리를 의미합니다!
업로드 과정에서 mainCategoryId(부모 카테고리 id), categoryId(서버에 저장되는 카테고리 id)를 둘 다 저장하고 있어요, 그땐 꼭 필요하다고 생각해서 둘 다 저장했는데... 지금 다시 보니까, categoryId만 저장하고 findParentCategory를 사용해도 될 것 같다는 생각이 들었숩니다....... 이 부분은 기록해두고, 수정 완료한 후에 리팩토링해보겠습니다!!

Comment on lines 43 to 51
const initialData = {
mainCategoryId: postData?.posts.categoryId ?? null,
categoryId: postData?.posts.categoryId ?? null,
title: postData?.posts.title ?? '',
content: postData?.posts.content ?? '',
isQuestion: postData?.posts.isQuestion ?? false,
isBlindWriter: postData?.posts.isBlindWriter ?? false,
images: postData?.posts.images ?? [],
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ask; 이거 잘 동작 하나요? useState 함수의 첫번째 인자로 들어가고 있는데, react query 에서 데이터가 캐싱되어있지 않으면 API 완료 후 값이 바뀌는게 무시될 것 같아요.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

초기 데이터를 쿼리하고 제출하는 부분과 실제 수정하고 제출하는 폼 부분을 분리하는게 어때요? 후자는 다음과 같은 인터페이스를 갖는거죠. 이렇게 한다면 API 호출 로직과 초기 값 채워주는 로직을 복잡하게 분기처리 할 필요가 없을 것 같아용

// 수정

const { data } = use초기값쿼리();

data != null && <FeedUploadForm initialData={data} onSubmit={() => mutateEdit()}/>
// 업로드

<FeedUploadForm onSubmit={() => mutateEdit()}/>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

호오오... 이 방법 생각을 못했는데, 좋다고 생각합니다!!!!


export default function FeedUploadPage({ isEdit, feedId = '' }: FeedUploadPageProp) {
const router = useRouter();
const { data: postData } = useGetPostQuery(feedId);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

p2; edit 중이 아닐 땐 유효하지 않은 API 요청이 나가버릴 것 같아요. react query의 enabled 같은 옵션으로 제어하는게 좋을 것 같네요

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FeedUploadPage 컴포넌트가 edit>[id].tsx, upload.tsx 이렇게 두군데에서 쓰이는데요, 그래서 해당 로직이 아예 FeedUploadPage 밖에서 이루어지도록 했습니다!
edit>[id].tsx에서는 const { data } = useGetPostQuery(feedId); API 요청이 있지만, upload.tsx에서는 아예 해당 훅을 부르지 않게 수정했어요!

Copy link
Member Author

@seojisoosoo seojisoosoo Jan 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

흐움움 근데 생각해보니까 feedId값이 undefined일때 잘못된 요청이 갈 수도 있을 것 같아서, enabled로 제어했습니다! feedId는 현재 어느 피드로 라우팅되었는지를 의미하는 값인데 이게 아직 안 읽혔으면 ""으로 보내고 있어요, ""이면 false로 인식하니까 enabled:!!feedId 이렇게 추가해주었습니다!

@seojisoosoo seojisoosoo requested a review from Tekiter January 17, 2024 10:17
Copy link
Collaborator

@juno7803 juno7803 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다! 남겨둔 리뷰들은 동작에 문제 없다면 머지하고 반영해도 괜찮을 것 같아요 😁

@@ -59,5 +59,6 @@ export const useGetPostQuery = (postId: string) => {
return useQuery({
queryKey: getPost.cacheKey(postId),
queryFn: () => getPost.request(postId),
enabled: !!postId,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postIdstring 타입으로 들어오는거면 항상 true 이지 않나요?!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

""이면 false로 인식하니까 enabled:!!feedId 이렇게 추가해주었습니다!

아 당연히 라우팅에서 params 못읽어오면 undefined 일 줄 알고 남긴 리뷰인데, string | undefined가 아니라, 사용처에서 "" 로 처리를 해주는군요,,

요걸 모르면 살짝 헷갈릴 수 있을 것 같아서,

  1. enabled 옵션을 useGetPostQuery에서 받아서, FeedUploadPage에 enabled 조건이 언제인지 알 수 있게 드러낸다.
  2. useGetPostQuery의 postId: string | undefined 그대로 받는다

둘 중 하나로 바꿔보는건 어떨까요? 지금 지수가 처리해둔 구조는 두 개의 파일을 모두 확인해야 이해가 빠를 것 같아서 제안해요!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호 동의합니다..!!
useGetPostQuery이 다른 컴포넌트에서도 사용되고 있어서, 1로 하게되면 다른 곳에도 enabled옵션을 추가해주어야할 듯 하여 2번 방법이 좋을 것 같다고 생각해요!
쿼리키나 함수에 undefined인 값을 넘겨줄 수 없으니까 처음부터 string값을 넘겨줘야겠다고 생각했는데, 남겨주신 리뷰 읽어보니 타입 그대로 받고 키랑 함수에 들어가는 id는 이렇게 넣어주면 되겠구나 싶어서 이렇게 수정해보았습니다!!

export const useGetPostQuery = (postId: string | undefined) => {
  const id = postId ?? '';

  return useQuery({
    queryKey: getPost.cacheKey(id),
    queryFn: () => getPost.request(id),
    enabled: !!postId,
  });
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

아주 굿 입니다!! 저는 id가 '' 이나 0 인게 약간 어색하다 생각해서 아래처럼 처리하는데요,
지수가 해준것처럼 해도 무관하긴 할 것 같네욥

if (postId == null) {
 throw new Error('id가 없습니다 어쩌고');
}

return useQuery({
  ...,
  enabled: postId != null
})

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

호오오 바로 에러를 던지는 방법도 있겠구만요!!


export default function FeedUploadPage({ initialForm, editingId, onSubmit }: FeedUploadPageProp) {
const router = useRouter();
const isEdit = editingId !== null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이렇게 하면 undefinednumber 타입 둘다 true가 되지 않나요?!
(editingId?: number | null 인 것 같아서 얘기해요!)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오엠쥐,,, 정말 매의 눈이십니다!!! 감사해요ㅜㅜ 이렇게 변경했습니다!

editingId?:number;
const isEdit = editingId !== undefined;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null 또는 undefined 를 처리하려면 다음과 같은 방법도 가능해용
editingId != null

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉! 넘 유용합니다!!

Copy link
Collaborator

@juno7803 juno7803 Jan 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

다음 comment로 정확히 커교수님이 말해준 사실을 적으려 했습니다 ㅎㅎ 굿굿

interface FeedUploadPageProp {
editingId?: number | null;
initialForm: UploadFeedDataType;
onSubmit: UseMutateFunction<unknown, Error, { data: FeedDataType; id: number | null }, unknown>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onSubmit의 args의 타입이 react-query의 mutate에 의존할 필요는 없다고 생각해요! (불필요하게 의존하게 되는 느낌)

initialForm 이라는 이름도 괜찮은데요! 저는 defaultValue 라고 좀 더 일반적으로 많이 쓰이는 이름을 선호합니다!! (just 참고..)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

오호..! 사실 onSubmit 타입을 이렇게 저렇게 추론을 많이 했었는데, 계속 저 타입이 아니라면서 에러가 뜨더라구요.. 혹시 의존하지 않고 선언할 수 있는 타입이 무엇이 있을까요??

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onSubmit 은 submit된 데이터만 전달하고, 사용하는 쪽에서는 한번 함수로 받아서 사용하면 될 것 같아요~

onSubmit: ({ data: FeedDataType; id: number | null }) => void;
<Comp onSubmit={(value) => mutate(value)} />

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굿굿

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

헉 대박 완전 이해했어요!!

@seojisoosoo seojisoosoo added this pull request to the merge queue Jan 21, 2024
Merged via the queue into main with commit ca2ed9e Jan 21, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: 피드 수정 구현
3 participants