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

(#247) slow 3G 환경에서도 #247이 여전히 재현되는 버그 수정 #263

Merged
merged 1 commit into from
Apr 8, 2023

Conversation

GooJinSun
Copy link
Collaborator

@GooJinSun GooJinSun commented Apr 1, 2023

Issue Number: #247

Self Check List

  • !!! PR이 머지되는 base branch을 꼭 확인해주세요 !!!
  • base branch로부터 rebase를 했나요?
  • main으로 머지되는 PR인 경우 릴리즈 노트를 작성해주세요

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Refactoring (formatting, renaming)
  • Merge Develop to Main
  • Other (please describe):

What does this PR do?

  • draft를 관리하는 resetContentcreatePost보다 먼저 실행해서 게시할 때도 draft로 저장되지 않도록 수정
  • /home으로 redirect되는 로직을 비동기 createPost가 완료된 이후 되도록 해서, 이전 캐싱된 feed를 보여주지 않도록 수정
    • 점점 더 (자주 바뀔 수 있는 여지가 있는) feed는 캐싱하면 안된다는 생각이 강해지네요ㅠ.ㅠ
  • refetch시에도 loading component를 보여줄 수 있도록 수정

isLoading is only true if you have a hard loading state where you have no data. When you are refetching, you most certainly have data already, which is why the query stays in success state. This is the stale-while-revalidate principle react-query is built upon.

What you are most likely looking for is the isFetching flag, which will also be true for background updates. 참고 isFetching vs isLoading

Preview Image

2023-04-02.8.34.35.mov

Further comments

bugfix: #247 resetContent before async createPost not to save draft on submit
@GooJinSun GooJinSun requested a review from koyrkr April 1, 2023 23:47
@GooJinSun GooJinSun self-assigned this Apr 1, 2023
@GooJinSun GooJinSun added bug Something isn't working frontend labels Apr 1, 2023
@GooJinSun GooJinSun added this to the 4월 스프린트 milestone Apr 1, 2023
Copy link
Collaborator

@koyrkr koyrkr left a comment

Choose a reason for hiding this comment

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

이 부분은 저번에 잠깐 이야기 나누었던 피드 캐싱 최소화 등등의 이슈들과 완전 맞닿아 있는 수정사항 같네요 ㅠㅠ feed 쪽에 괜히 먼저 react-query를 적용해봤나 생각도 들고요..

일단 버그 수정은 되어야하니 이 부분은 고대로 반영되어도 좋을 것 같고, 캐싱 관련 부분은 어떻게 할지 전략을 잘 짜보고 같이 수정해보면 좋을 것 같네요!!!

감사합니다~! ☺️

Comment on lines +69 to +72
// FIXME: feed는 캐싱하면 의도치 않게 stale한 응답을 보여줄 수 있음
refetchFriendPostList({ refetchPage: () => true });

if (location.pathname === '/questions') history.push('/home');
Copy link
Collaborator

Choose a reason for hiding this comment

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

어차피 home으로 redirect 시켜주고 refetch를 할 거면
useInfiniteFriendPostList 안에서
refetchOnMount: true
option을 주는 건 어떨까요?

feed는 말씀대루 특성상 캐싱이 되는 것을 최소화하는게 맞는 것 같고,
고러면 refetchFriendPostList도 /home이 아닌 곳에서 굳이 할 필요 없지 않나 싶어서요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

네! 이건 오늘 얘기한 scroll 유지 기능 스펙아웃 검토하면서 같이 검토해보기로 해요~~
캐시할 필요가 없어지만 지나님 말씀대로 refetchOnMount되도 좋을 것 같습니다!

@GooJinSun GooJinSun merged commit 9adde2b into main Apr 8, 2023
@GooJinSun GooJinSun deleted the bugfix/issue-247 branch April 8, 2023 02:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working frontend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] 답변 작성 시 draft에 저장이 되고 익명피드에도 공개됨
2 participants