Clear page cache on resetPages and page key changes #269

innocentiv · 2020-02-16T22:37:30Z

Adding a Reset for pages

based on the work of patrickus and gabrielperales@8ba8379

This is a copy of the Pull Request from patrickus
with the following changes:

reset pageCacheMap to ensure offset is computed after a reset is performed
restore types
restore JSX for page components (we need better eslint config to avoid warnings, pr coming)
remove unneeded useMemo call on pageCountKey and pageOffsetKey (strings are primitive values)

resolving: #189

Sorry for the duplicated PR, I needed those fixes for today so I created a fix on my repo. Disregard this PR if patrickus want to improve the work done on his PR.

thanks to https://github.com/patrickus and https://github.com/gabrielperales

gabrielperales · 2020-03-06T16:22:22Z

src/use-swr-pages.tsx

@@ -205,7 +219,7 @@ export function useSWRPages<OffsetType = any, Data = any, Error = any>(
      p.push(pageCache[i].component)
    }
    return p
-  }, [_pageFn, pageCount, pageSWRs, pageOffsets, pageKey])
+  }, [_pageFn, pageCount, pageSWRs, pageOffsets, pageKey, pageOffsetKey])


@innocentiv pageOffsetKey will always change with pageKey... so it is not needed to put it here as a dependency, is it needed?

It is not actually needed, but I think the recommended approach is to add all the values used inside the effect as a dependency to avoid stale closure pitfalls. There is none in this case, thanks to the link between pageKey and pageOffsetKey , but I think it helps with code clarity and in future refactor. The link between these two values is elsewhere in the codebase, and the is no problem in adding a dependency. In general, there are lots of missing dependencies in swr. The issue was also discussed here: #263 . We should try to decide the best way to manage dependencies in hooks.

More info here: facebook/react#14920 (comment)
dan abramov blog: https://overreacted.io/a-complete-guide-to-useeffect/#dont-lie-to-react-about-dependencies
react docs: https://reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies

I see... Thanks for clarifying it. Let's see if any of the reviewers decides to check the code soon. I've been waiting a lot this feature to be merged!

innocentiv · 2020-03-17T11:52:56Z

Hi, reviewers. It has been a month, could I ask you why nobody is reviewing this MR?
Right now the pagination is broken if you are changing the key to get a new page list.
If there is any issue, or you think that this is not a good solution, just tell us what to do.

muyiwaoyeniyi · 2020-03-30T04:40:46Z

@innocentiv Thanks for this fix. I will use your branch until the fix is official.

Thanks.

muyiwaoyeniyi · 2020-03-30T05:20:29Z

@innocentiv Maybe consider clearing out the cache for each page swr in resetPages..

sanderfish · 2020-03-30T08:04:05Z

Would love to see this merged.

Edit: @quietshu just realised where you are. Hope you're ok, wishing you the best!

muyiwaoyeniyi · 2020-03-30T14:13:08Z

@innocentiv I'm having issues installing/building the project. After installing the repo with npm, I'm unable to reference 'swr' and when I try building it myself, I run into (I guess typescript type) errors. I will appreciate any help with this.

Thanks.

innocentiv · 2020-03-30T14:53:02Z

@muyiwaoyeniyi This branch does no contain the build artifact for swr. I have created a branch https://github.com/innocentiv/swr/tree/reset-pages-build with a build for use in my projects. This build was made some time ago, on an older release and I cannot guarantee there would be no regression. You could try it with:

npm install github:innocentiv/swr#reset-pages-build

muyiwaoyeniyi · 2020-03-30T14:58:44Z

@innocentiv Got it. Thanks.

gabrielperales · 2020-04-16T11:09:38Z

Jack-Works · 2020-04-26T04:30:36Z

Using this branch, the stale offset bug is fixed, thanks!
But there are still strange bugs, I try to debug the hooks state

The final array is offset array.

As you can see, when switch from old page key suj to new page key suji, there is a moment (Bad state 1 in the picture) that old offsets is mixed with new pageKey.

And in the bad state 2, for unknown reason, the page count increased (but I only have 1 page of data), and useSWRPages start to return duplicated data.

Green box: Original data
Yellow box: Duplicated data (I believe it's coming from offset[1])
Red box: Duplicated data (I believe it's coming from offset[2])

This bug is not stable, depends on how I type the search query so I believe it's some kind of race condition.

Jack-Works · 2020-04-26T04:41:08Z

(I'm using useSWRPages for infinite loading so the loadMore function is called automatically, therefore it's easier to trap the race condition bug)

innocentiv · 2020-04-29T18:07:07Z

@Jack-Works and @muyiwaoyeniyi thank you for testing this PR. I think more unit tests are needed. I will be working on this as soon as possible.

muyiwaoyeniyi · 2020-04-29T18:37:39Z

@innocentiv My pleasure. @Jack-Works I've seen that issue and I also have an infinite loading implementation with react-window so it all works out.

Jack-Works · 2020-04-29T19:30:21Z

@muyiwaoyeniyi oh how do you use it with react-window? Can you share an example? Maybe I did something wrong therefore having bugs in it.

muyiwaoyeniyi · 2020-04-30T03:12:04Z

@Jack-Works Certainly. I can put chunks of my code on codesandbox. That would be the quickest option for me. It would take some time to cut it down to highlight the key parts. As a side note, I normalize all responses from my api so it inherently removes duplicates.

Let me know if that works and I'll put it up.

Thanks.
Taiwo

muyiwaoyeniyi · 2020-04-30T13:44:19Z

@Jack-Works Here's my code.

https://codesandbox.io/s/holy-sunset-67b5v?file=/src/App.js
The files are App.js -> roles.js -> infiniteloader.js

Happy to explain what's going on. Let me know.

ftes · 2020-05-13T21:16:52Z

Any help needed to move this PR forward?

ftes · 2020-05-13T21:56:15Z

src/use-swr-pages.tsx

+  }, [pageOffsetKey, pageCountKey])
+
+  // Reset page if the key changes
+  useEffect(() => resetPages(), [resetPages])


Shouldn't deps be used as the dependencies for this effect?

As also done originally:
https://github.com/gabrielperales/swr/commit/8ba8379d2e6c580ded234385d34ea829be996788#diff-c9d2eb3ea821da85bd697b22fe31edf4R141

deps itself can't be a dependency cause it is different everytime. You can't spread it either cause react will complain dependency array can't be different length during different render.

I agree deps itself cannot be a dependency, but I also agree with ftes in saying that I could spread it alongside resetPages (I expect the user to provide a dependency array with the same size every render). Another issue is if we need to reset the pages every time a value used inside the Page component gets updated. In the original API, there is no deps array for the pages, so I use the pages key as a dependency. Maybe we need some clarification about the deps argument from the code owner.

CurtisBaldwinson · 2020-05-30T14:20:25Z

Just wanted to let you know that there is still occasionally a stutter on this where the data duplicates once. It's still much better than the click*x number of duplication that happened without this PR.

If anyone is interested in the full fix (acknowledging the awesome work you did here, I'm sure we all really appreciate your help here), there is a PR at #404 that completely addressed the issue (at least for my use case) and with just a few lines of code.

Perhaps it's worth comparing notes on the techniques here and there may be an even more optimal way to achieve this?

shuding · 2020-07-25T16:59:12Z

I think we can close this due to the new useSWRInfinite API: #435. It should have solved this problem (and other related ones). As always, thank you for your awesome contribution! 🙏

#243 (comment)

We've been getting a lot of feedback about the old pagination API, and that's why we didn't put it on the documentation publicly and decided to work on a completely new API.

The new API is way more stable and easier to use. We will update the docs once the stable version is released.

innocentiv requested review from pacocoursey, shuding and Timer as code owners February 16, 2020 22:37

innocentiv mentioned this pull request Feb 16, 2020

adding resetPages to clear cached pages on new searches #243

Closed

innocentiv force-pushed the reset-pages branch from 171db42 to 57b9cf2 Compare February 27, 2020 08:28

gabrielperales suggested changes Mar 6, 2020

View reviewed changes

gabrielperales approved these changes Mar 6, 2020

View reviewed changes

muyiwaoyeniyi mentioned this pull request Mar 30, 2020

Is there a way to reset pages of useSWRPages? #189

Closed

justsee added a commit to otsu-app/swr that referenced this pull request Apr 15, 2020

Apply vercel#269.

516cb07

innocentiv force-pushed the reset-pages branch from 57b9cf2 to 2fecc13 Compare April 16, 2020 14:15

ftes reviewed May 13, 2020

View reviewed changes

CurtisBaldwinson mentioned this pull request May 30, 2020

Refresh state when change pageKey #404

Closed

fix: add reset function to swr pages

310f87f

innocentiv force-pushed the reset-pages branch from 2fecc13 to 310f87f Compare June 2, 2020 18:13

shuding mentioned this pull request Jun 9, 2020

New infinite loading API #435

Merged

7 tasks

shuding closed this Jul 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear page cache on resetPages and page key changes #269

Clear page cache on resetPages and page key changes #269

innocentiv commented Feb 16, 2020

gabrielperales Mar 6, 2020

innocentiv Mar 6, 2020

gabrielperales Mar 6, 2020

innocentiv commented Mar 17, 2020

muyiwaoyeniyi commented Mar 30, 2020

muyiwaoyeniyi commented Mar 30, 2020

sanderfish commented Mar 30, 2020 •

edited

muyiwaoyeniyi commented Mar 30, 2020

innocentiv commented Mar 30, 2020

muyiwaoyeniyi commented Mar 30, 2020

gabrielperales commented Apr 16, 2020

Jack-Works commented Apr 26, 2020

Jack-Works commented Apr 26, 2020

innocentiv commented Apr 29, 2020

muyiwaoyeniyi commented Apr 29, 2020

Jack-Works commented Apr 29, 2020

muyiwaoyeniyi commented Apr 30, 2020

muyiwaoyeniyi commented Apr 30, 2020

ftes commented May 13, 2020

ftes May 13, 2020

Jack-Works May 14, 2020

innocentiv May 14, 2020 •

edited

CurtisBaldwinson commented May 30, 2020

shuding commented Jul 25, 2020

Clear page cache on resetPages and page key changes #269

Clear page cache on resetPages and page key changes #269

Conversation

innocentiv commented Feb 16, 2020

gabrielperales Mar 6, 2020

Choose a reason for hiding this comment

innocentiv Mar 6, 2020

Choose a reason for hiding this comment

gabrielperales Mar 6, 2020

Choose a reason for hiding this comment

innocentiv commented Mar 17, 2020

muyiwaoyeniyi commented Mar 30, 2020

muyiwaoyeniyi commented Mar 30, 2020

sanderfish commented Mar 30, 2020 • edited

muyiwaoyeniyi commented Mar 30, 2020

innocentiv commented Mar 30, 2020

muyiwaoyeniyi commented Mar 30, 2020

gabrielperales commented Apr 16, 2020

Jack-Works commented Apr 26, 2020

Jack-Works commented Apr 26, 2020

innocentiv commented Apr 29, 2020

muyiwaoyeniyi commented Apr 29, 2020

Jack-Works commented Apr 29, 2020

muyiwaoyeniyi commented Apr 30, 2020

muyiwaoyeniyi commented Apr 30, 2020

ftes commented May 13, 2020

ftes May 13, 2020

Choose a reason for hiding this comment

Jack-Works May 14, 2020

Choose a reason for hiding this comment

innocentiv May 14, 2020 • edited

Choose a reason for hiding this comment

CurtisBaldwinson commented May 30, 2020

shuding commented Jul 25, 2020

sanderfish commented Mar 30, 2020 •

edited

innocentiv May 14, 2020 •

edited