Fix: issue 4485, check 4484 #4508
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| if (isLoadingThreads && !isRefetched) { | ||
| return <Loading /> | ||
| } | ||
|
|
There was a problem hiding this comment.
Great work on catching this ! It wasn't an easy one to debug 🤯
But I think the fix could be improved because first it now makes empty categories show a loader every 6 seconds e.g: https://dao-git-fork-mkbeefcake-fix-4485-4484-joystream.vercel.app/#/forum/category/17?network-config=https://34.230.5.182.nip.io/network/config.json. But more importantly it still refetch way to much data (as you pointed out on #4484). Also I realized we've struggled with this particular useRefetch for a long time (see #4318) so I really think the best is to remove this one.
Instead we could replace this logic by a pollInterval of 6 second in packages/ui/src/forum/hooks/useForumCategoryThreads.ts but only when isArchive is false. Also isLoading should become !threadsData || !countData this way isLoading is only true on the initial load.
There was a problem hiding this comment.
Thanks for your guide. also I figured out "posts" field in forumThreads query, If i choose this mechanism, i can reduce the query count to 1, but it could take all posts info per each thread, so could take much data and a bit more loading time.
I hope to get your opinion about this.
There was a problem hiding this comment.
Yes forumThreads { posts { ... } } will return all the posts for all the threads shown one the category page so I think 1 request per thread is a better trade off.
There was a problem hiding this comment.
@thesan I have added posts field for ForumThread interface and fetched it in GetForumThreads Query, so reduce the queries in Forum Category page, please check the updated code.
mkbeefcake
force-pushed
the
fix-4485-4484
branch
from
9b1d6f1
to
04dd258
Compare
| @@ -42,7 +42,7 @@ export const ThreadList = ({ | |||
| return <NotFoundText>No threads found</NotFoundText> | |||
| } | |||
|
|
|||
| if (isLoading) { | |||
| if (isLoading && threads.length <= 0) { | |||
There was a problem hiding this comment.
Hey, please reverse the changes to this file it's not needed anymore
| posts { | ||
| id | ||
| createdAt | ||
| isVisible | ||
| text | ||
| authorId | ||
| edits { | ||
| createdAt | ||
| } | ||
| status { | ||
| __typename | ||
| } | ||
| threadId | ||
| thread { | ||
| categoryId | ||
| } | ||
| author { | ||
| ...MemberFields | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't think we should go with this solution:
ATM on the Joystream General category it ~6x the size of the thread requests from 137kb to 851kb but the 30 posts request saved by this solution were between 4kb to 10kb so already the page queries a lot more data to start with. And then it repeats the thread request every 6 seconds. On top of that the thread request go all the way to several megabytes on some pages.
A bigger request can often be better than a lot of smaller one but here I really don't think this is worth it.
There was a problem hiding this comment.
I have removed "posts" from thread and updated the code. please review and give me feedback
mkbeefcake
force-pushed
the
fix-4485-4484
branch
from
328ca9a
to
21abeb6
Compare
mkbeefcake
force-pushed
the
fix-4485-4484
branch
from
8b049bc
to
324f845
Compare
Fixed #4485 and checked #4484