Error and retry support for infinite scroll#30964
Conversation
Now it only exposes one fetch function that can be used to trigger fetches from infinite scroll, regardless of whether it's the initial or a subsequent fetch. Restarting the data stream is driven by changing filtering parameters.
|
Seems reliable from local testing and I haven't been able to reproduce the bug you were talking about in a real-use scenario. Will go over the code again shortly |
ibeckermayer
left a comment
ibeckermayer
left a comment
There was a problem hiding this comment.
Looking very close, just a final consideration re: useLayoutEffect vs the setState approach.
| // cause us to display incorrect data. This can be reproduced by rapidly | ||
| // changing filtering data on the resources list page. |
There was a problem hiding this comment.
| // cause us to display incorrect data. This can be reproduced by rapidly | |
| // changing filtering data on the resources list page. | |
| // cause us to display incorrect data. (This issue can be reproduced by switching this to | |
| // `useEffect` and rapidly changing filtering data on the resources list page). |
There was a problem hiding this comment.
I notice that we're using useLayoutEffect here for what seems to be an identical reason to why we're using this approach in useKeyBasedPagination. Is there a simple way that we can modify one or the other to use a single approach for achieving this effect? Related question: is there particular reason to generally prefer one over the other?
There was a problem hiding this comment.
I don't know. Perhaps there is a better, simpler approach, but I think I reached a point of diminishing returns here with attempting to make this code as clear and robust as possible. I don't really remember why exactly, it was "just the way I did it".
There was a problem hiding this comment.
Related question: is there particular reason to generally prefer one over the other?
I found this post to be useful. https://kentcdodds.com/blog/useeffect-vs-uselayouteffect
There was a problem hiding this comment.
@bl-nero If you don't know whether your design makes sense yet then feel free to convert this to a draft. Otherwise, come back to it and figure out the actual answer, I don't accept your apathetic response.
Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
ibeckermayer
left a comment
ibeckermayer
left a comment
There was a problem hiding this comment.
See #30964 (comment)
* Add some unit tests to the existing useInfiniteScroll hook * Change the interface of the infinite scroll hook Now it only exposes one fetch function that can be used to trigger fetches from infinite scroll, regardless of whether it's the initial or a subsequent fetch. Restarting the data stream is driven by changing filtering parameters. * Harden useKeyBasedPagination * Install jsdom-testing-mocks * Error and retry support for infinite scroll * Fix type-check * Fix a state management bug * Make useInfiniteScroll a wrapper over useKeyBasedPagination * Get rid of unnecessary useEffect * Address review comments * Layout fixes * Review * Lint fix * Add the license * One more review round * Tweak the comment in useInfiniteScroll Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com> --------- Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
* Add some unit tests to the existing useInfiniteScroll hook * Change the interface of the infinite scroll hook Now it only exposes one fetch function that can be used to trigger fetches from infinite scroll, regardless of whether it's the initial or a subsequent fetch. Restarting the data stream is driven by changing filtering parameters. * Harden useKeyBasedPagination * Install jsdom-testing-mocks * Error and retry support for infinite scroll * Fix type-check * Fix a state management bug * Make useInfiniteScroll a wrapper over useKeyBasedPagination * Get rid of unnecessary useEffect * Address review comments * Layout fixes * Review * Lint fix * Add the license * One more review round * Tweak the comment in useInfiniteScroll --------- Co-authored-by: Isaiah Becker-Mayer <isaiah@goteleport.com>
5 participants
This PR hardens the existing hook, adds a bunch of tests, changes its API, and adds an error&retry UX on top of it.
Demo: https://www.loom.com/share/1b23d99dc6454f7d9f78380e17a81328?sid=d73d6915-fae7-482b-a578-157ee4259b28