adopted-ember-addons / ember-infinity

:zap: Simple, flexible Infinite Scroll for Ember CLI Apps.
MIT License
377 stars 131 forks source link

Bi-Directional Infinite Scrolling Pattern Limitations #293

Open johncalvinyoung opened 6 years ago

johncalvinyoung commented 6 years ago

So as I work on a branch for #292 (and the feature that gave rise to it) I've run into some thought problems.

I'm working with an app using non-linear paging (keyset pagination rather than limit&offset), heavily using the new subclass of InfinityModel pattern, and the next feature is likely to use bidirectional loading with two InfinityLoader instances, as outlined in the readme.

Anyone have thoughts on the above? Some of this would affect interfaces somewhat...

snewcomer commented 6 years ago

@johncalvinyoung

  1. Now that we have the infinity service, does that solve your use case? Basically can be injected anywhere now.
  2. Do you have a concrete example of this? You have to have afterInfinityModel when loading the records from the page before? I had thought _afterInfinityModel was always called, no matter which direction.

Would love to help/collaborate on any work you may have up for the problem you are solving.

Have this existing PR so far - #287

johncalvinyoung commented 6 years ago

@snewcomer

  1. The infinity service actually cannot be injected into the extended InfinityModel. Some complaint about not having a container. I ended up dropping it at that point and just setting it on the model in the first _afterInfinityLoad hook for now. Would be better when the infinity service sets up the model.

  2. I'm using a highly-modified form of the 'cursor pagination' from the readme. This logic is used when you cannot extrapolate the availability of records from a (not present) page number or total page number. I'm in the middle of implementing bidirectional infinite scrolling, and I'll need to track 'oh you reached the end of the result set' ==> .set('canLoadMore', false) differently from 'oh, you reached the beginning of the result set' ==> set('canLoadMoreBefore', false)? In this specific scenario, we'll be sometimes launching the route with some initial parameters to start loading at a specific time or sequence number in the middle of a very long set or queue.

    afterInfinityModel(posts, infinityModel) {
    let loadedAny = posts.get('length') > 0;
    infinityModel.set('canLoadMore', loadedAny);
    }

I'll see if I can push up the hook work I was doing this afternoon. I'm pretty sure I've got the semantics of some of the promises wrong, though. And I'm struggling a bit with extending the test suite in the library--very different patterns/techniques than my projects have.

johncalvinyoung commented 6 years ago

PS: I've been reading #287 as it goes along--it'll be good improvements when we get there, as long as those of us who don't use paged APIs can still get along!

snewcomer commented 6 years ago

@johncalvinyoung

  1. Does this commit help?

  2. The goal of 1.0 is to be extensible as possible. If it is too difficult with the existing apparatus, we should brainstorm on a new way to handle cursor pagination.

johncalvinyoung commented 6 years ago

If that commit works I'll be surprised. That's basically exactly what I attempted to do in my app, in the subclass.

Edited to note: the exception was thrown once I attempted to use the service inside the class.

johncalvinyoung commented 6 years ago

Update: I'm using 1.2.4 now as I picked back up my feature branch in progress. Lots of good changes there.

However, to combine bidirectional loading (which can even be concurrent) with cursor/keyset pagination correctly required a number of adaptations to both the infinityModel and infinityService classes (subclassed them in our app). I'll be looking at our changes to figure out if there's a good abstraction I can push back upstream. One of the key concepts is that we can't rely on a single currentPage or _increment attribute on the infinityModel, as the various hooks may update those values out of order in a race condition scenario. Passing increment around into most method calls instead of relying on state in the infinityModel helped. And the concepts of _canLoadMore and _loading and reachedInfinity are all more-or-less unidirectional in nature, and for optimum extensibility ought to be split into before/after above/below forms in the superclass.

johncalvinyoung commented 5 years ago

Ooh. Next problem. Race condition on _increment will result in the previous page loading at the end, or vice versa. I think this would affect numbered APIs as well as keyset-paginated, if loading bidirectionally.