mharris717 / ember-cli-pagination

Pagination Addon for Ember CLI
MIT License
272 stars 114 forks source link

Upgrade to 2.0.x causes error #146

Open typeoneerror opened 9 years ago

typeoneerror commented 9 years ago

Not sure if there is something to be done in pagination, but at the moment it looks incompatible with Ember 2.0.x because of an issue with query params:

https://github.com/emberjs/ember.js/issues/11592

If I use the *Binding methods I run into this error.

DylanLukes commented 9 years ago

+1, will update with more as I investigate this.

broerse commented 9 years ago

@mharris717 and I like to update to 2.0 soon. @DylanLukes if you switch to 2.0 are there errors in ember test also?

DylanLukes commented 9 years ago

The issues I'm having seem to be related to query parameters and binding, as discussed in the linked issue. This isn't due to a bug so much as depending on behavior no longer supported by Ember. And to my understanding, it wasn't officially supported in the first place, it just happened to work.

Here's my understanding of what's happening:

1) Route returns model, router.js calls into Controller#setup (excerpt below). 2) Since there is a transition (from '' to '<your-model>.index') it also updates the values on which the model depends, in this case query params. 3) For some reason, the cache is found, so it iterates over the query params stored in it and sets them on the controller. 4) When it attempts to set query parameters on the controller it triggers Controller#_qpChanged (this is the step that is related to using bindings for query params on the controller, I believe). 5) However, _qpChanged depends on _qpDelegate being set, and that doesn't happen until the end of the first excerpt.

Hope this helps. I'm still actively investigating currently.

Except 1:

    setup: function (context, transition) {
      var controller;

      var controllerName = this.controllerName || this.routeName;
      var definedController = this.controllerFor(controllerName, true);

      if (!definedController) {
        controller = this.generateController(controllerName, context);
      } else {
        controller = definedController;
      }

      // Assign the route's controller so that it can more easily be
      // referenced in action handlers. Side effects. Side effects everywhere.
      if (!this.controller) {
        var propNames = _emberMetalProperty_get.get(this, '_qp.propertyNames');
        addQueryParamsObservers(controller, propNames);
        this.controller = controller;
      }

      var queryParams = _emberMetalProperty_get.get(this, '_qp');

      var states = queryParams.states;
      if (transition) {
        // Update the model dep values used to calculate cache keys.
        _emberRoutingUtils.stashParamNames(this.router, transition.state.handlerInfos);

        var params = transition.params;
        var allParams = queryParams.propertyNames;
        var cache = this._bucketCache;

        allParams.forEach(function (prop) {
          var aQp = queryParams.map[prop];

          aQp.values = params;
          var cacheKey = _emberRoutingUtils.calculateCacheKey(aQp.prefix, aQp.parts, aQp.values);

          if (cache) {
            var value = cache.lookup(cacheKey, prop, aQp.undecoratedDefaultValue);
            _emberMetalProperty_set.set(controller, prop, value);
          }
        });
      }

      controller._qpDelegate = states.allowOverrides;

      if (transition) {
        var qpValues = getQueryParamsFor(this, transition.state);
        controller.setProperties(qpValues);
      }

      this.setupController(controller, context, transition);

      this.renderTemplate(controller, context);
    },

Except 2:

_qpChanged: function (controller, _prop) {
      var prop = _prop.substr(0, _prop.length - 3);

      var delegate = controller._qpDelegate;
      var value = _emberMetalProperty_get.get(controller, prop);
      delegate(prop, value);
    },
DylanLukes commented 9 years ago

Good news everyone.

I have a fix. It's a little involved but can be integrated pretty easily. I'm nailing down the details. The changes are roughly:

1) remove pageBinding/perPageBinding from controller. You still need the totalPage binding and that's totally kosher. 2) do not update PagedRemoteArrays page/perPage. Instead, add actions to your router that modify query parameters and force a reload of the model ('refreshModel: true' on query params on route)

To summarize, changes in page/perPage should propagate from the router down via model refreshes. Changes in the page/perPage parameters should not be the concern of the Controller, but the Router, which should update them based on actions.

jcope2013 commented 9 years ago

:+1:

DylanLukes commented 9 years ago

Currently rewriting a new PaginationComponent. The controller/route mixins aren't strictly necessary...

mharris717 commented 9 years ago

Anyone have interest in pairing with me on this? I apologize for being a shit maintainer.

@jcope2013 @DylanLukes @broerse

DylanLukes commented 9 years ago

Unfortunately, I can't currently take up maintaining anything.

1) I'm no Ember expert; I'm more of a compilers/PLT/systems-level guy :). 2) I'm currently an undergrad, applying to graduate schools. Ergo, no free time.

It looks like for the most part it should be possible to reimplement this library very minimally, though. I hope my samples will demonstrate the right direction.

mharris717 commented 9 years ago

@DylanLukes just looking for somebody to work with me on a single pairing session.

Thanks for all your work already.

DylanLukes commented 9 years ago

Ah okay, I don't actually know what a pairing session is :). I haven't been formally trained in software engineering. I'm a programmer/mathematician, but I'm not a software developer. I just write web apps and infrastructure for my part-time job.

I can tell you all about the finite complement topology, but you'd lose me at "agile".

mharris717 commented 9 years ago

@DylanLukes based on your comments here you seem to know what you're doing :)

Was looking for somebody to screenshare with and work through getting this upgraded to 2.0 together. Apparently I can't muster up the activation energy on my own.

DylanLukes commented 9 years ago

I may be able to later on, but probably not any time soon. Graduate applications have consumed my life.

From the looks of it though, all that's really necessary is a nice RouteMixin and a PaginationComponent. In general, I think the PagedArray classes can go, that state management should probably be kept in the route.

The data flow should be:

actions (nextPage, prevPage) => linkTo/transitionTo (e.g. from PaginationComponent) => Router query params are updated (and therefore the controller ones) * => refreshModel => Controller#model => Controller#setupController: setAttributes on PaginationComponent (and display Component or setModel on a View for legacy)

Basically, propagate data downwards only. The controller just needs a dumb model with the current page. The other state (currentPage, etc) can be passed down in an opt-in manner via separate attributes on the component/view displaying the paginated content.

tl;dr: Model loading is a Router concern, and Pagination is a dependency of Model loading, so it's also a Router concern.

mharris717 commented 9 years ago

The PagedArray classes were my attempt to keep some OOP sanity, instead of spreading the logic all around the route. It might be too much trouble, I don't know.

DylanLukes commented 9 years ago

I see that, and for the most part that works great. It's just Ember is moving away from fat models :.

What I'm thinking for a rewrite (I may just do one myself) is:

(Side note: I'd probably prefer limit/offset over perPage/page/... but I use Django so that's unsurprising ;))

Here's a (non-functioning) quick sketch of how I see this working:

controllers/widgets.js

import Ember from "ember";

export default Ember.Controller.extend({
  page: 1,
  perPage: 5,

  queryParams: ["page", "perPage"]
});

routes/widgets.js

import Ember from "ember";

export default Ember.Route.extend({
  queryParams: {
    page: {refreshModel: true},
    perPage: {refreshModel: true},
  },

  model: function(params) {
    return Ember.RSVP.hash({
      content: this.store.find("widget", {
        page: params.page || 1,
        per_page: params.perPage || 10
      }
    });
  },

  setupController: function(controller, models) {
    controller.setProperties(models);
  },

  actions: {
    nextPage: function() {
      this.transitionTo('widgets', {page: this.modelFor('widget').get('meta.page') + 1});
    },
    prevPage: function() {
      this.transitionTo('widgets', {page: this.modelFor('widget').get('meta.page') - 1});
    },

    createWidget: function() {
      var newWidget = ...;
      // ...
      this.transitionTo('widget', newWidget.save());
    },

    saveWidget: function() {
      this.modelFor('widget').save();
    },

    deleteWidget: function() {
      this.modelFor('widget').destroyRecord().then(function() {
        this.transitionTo('widgets');
      }.bind(this));
    }
  }
});

templates/widgets.hbs

{{#widgets-master widgets=content page=content.meta.page ... }}
{{#pagination-thingy page=content.meta.page ...}}

We can override extractMeta on the Route to replace the old paramMapping and support different parameter schemes. :smile:

DylanLukes commented 9 years ago

Note this is agnostic to the pagination style. For remote pagination you'd do it like that, and for local pagination you do the same thing but omit the parameters in find() and instead define a "pagedContent" property on the controller.

Remote pagination happens in the router , local happens in the controller via computed properties.

Metadata management, instead of being placed on the content itself via a PagedArray, can just use Embers built in metadata management.

jcope2013 commented 9 years ago

@DylanLukes why is the

setupController: function(controller, models) {
    controller.setProperties(models);
  },

needed? or what is the intention of that? I am sort of confused there

DylanLukes commented 9 years ago

Oh, that's just a general practice. The assumption is that one route may need to load multiple models or sets of models to render disparate components. It's not really related to this issue, it's just a stylistic choice.

You could do for instance:

  model: function(params) {
    return Ember.RSVP.hash({
      content: this.store.findAll('widget'), // the 'primary' model, aliased to 'model'
      gadgets: this.store.findAll('gadget'),
    });
  },

  ...

  setupController: function(controller, models) {
    controller.setProperties(models);
  },

  // now we just have to be explicit and use this.controllerFor and this.modelFor throughout
  // get('controller') or get('model') will return the ones for 'content'.

, and then (using a barebones controller as before) we have a template like:

<div id="content">
  {{#widget-thing widgets=content}}{{outlet}}{{/widget-thing}}
</div>
<div id="sidebar">
  {{#gadget-aux-thing gadgets=gadgets}}
</div>

But gadgets is in scope here for auxiliary components, and we can access belongsTo and hasHany relations inside these components (e.g. someWidget.get('usedIn') => [Gadget, Gadget, ...] or gadget.get('primaryWidget') => Widget) without implicit scope violations. It's just, I think, more semantically correct and cleaner.

DylanLukes commented 9 years ago

Working on something for my own project. I'll factor it out into https://github.com/DylanLukes/ember-cli-paged for reference and posterity. I don't intend on maintaining it too actively, as pagination will be hitting ember-data. See:

https://github.com/emberjs/data/issues/2905 https://github.com/emberjs/data/issues/3048

However, for the time being, the pagination aspect of JSON API is not fully pegged down, and the pagination in ember-data will specifically be for JSON API, and not for other pagination schemes (Django Rest Framework uses limit/offset for instance).

Currently I have some bespoke additions to Django Rest Framework to render JSON API as well as handle some basic side loading, but it's not even remotely a complete JSON API implementation. It's a bit of a Frankenstein. I'd rather see more flexibility on the client side than forcing servers to use one particular structure, but oh well, ember-data is a convention-driven project, and it's pretty nice anyhow.

DylanLukes commented 9 years ago

Here's some snippets for those dealing with issues. I've managed to remove the need for server side pagination, at least for now. It might not be perfect yet, and notably the template is incomplete and doesn't provide gaps or sliding windows. How you render your page numbers is up to you.

This code handles pagination for a full set of available objects. It handles orphans also. It's a shameless port of Django's pagination. You can easily modify this to include gaps and modify the rendering. I've also implemented Ember.Array on these components as well.

As for chunking/lazy/loading data, it seems like the easiest way to do that here is to manually trigger feeding the store and passing that down to the component via the objects tree. That should cascade downward to provide the additional pages.

I tried to keep the code straightforward by abusing computed properties.

/pods/better-page-numbers/component.js

import Ember from 'ember';

var Page = Ember.Object.extend(Ember.Array, {
    objects: [],
    pageNumber: null,
    paginator: null,

    hasNext: Ember.computed.lt('pageNumber', 'paginator.pageCount'),

    hasPrev: Ember.computed.gt('pageNumber', 1),

    hasOther: Ember.computed.or('hasPrev', 'hasNext'),

    nextPageNumber: function () {
        return this.get('paginator').validatePageNumber(this.get('number') + 1);
    }.property('paginator', 'pageNumber'),

    prevPageNumber: function () {
        return this.get('paginator').validatePageNumber(this.get('number') - 1);
    }.property('paginator', 'pageNumber'),

    startIndex: function () {
        if (this.get('paginator.count') === 0) {
            return 0;
        }
        return (this.get('paginator.perPage') * (this.get('pageNumber') - 1)) + 1;
    }.property('paginator.objectCount', 'paginator.perPage', 'pageNumber'),

    endIndex: function () {
        if (this.get('pageNumber') === this.get('paginator.pageCount')) {
            return this.get('paginator.objectCount');
        }
        return this.get('pageNumber') + this.get('paginator.perPage');
    }.property(''),

    // Page implements Ember.Array for objects
    length: Ember.computed.alias('objects.length'),
    objectAt: function (idx) {
        return this.get('objects').objectAt(idx);
    }
});

var SimplePaginator = Ember.Object.extend(Ember.Array, {
    objects: Ember.computed.oneWay('parent.objects'), // the set of all paginated objects
    perPage: Ember.computed.oneWay('parent.perPage'),
    orphans: Ember.computed.oneWay('parent.orphans'),  // orphans: the minimum number of items allowed on the last page.
    allowEmptyFirstPage: Ember.computed.oneWay('parent.allowEmptyFirstPage'),

    objectCount: Ember.computed.alias('objects.length'),

    validatePageNumber: function (number) {
        if (number % 1 !== 0) {
            throw new Ember.Error('That page number is not an integer');
        }

        if (number < 1) {
            throw new Ember.Error('That page number is less than 1');
        }

        if (number > this.get('pageCount')) {
            if (number !== 1 || !this.get('allowEmptyFirstPage')) {
                throw new Ember.Error('That page contains no results.');
            }
        }

        return number;
    },

    page: function (number) {
        var beginIndex = (number - 1) * this.get('perPage'),
            endIndex = beginIndex + this.get('perPage');

        if (beginIndex + this.get('orphans') >= this.get('objectCount')) {
            endIndex = this.get('objectCount');
        }

        return Page.create({
            pageNumber: this.validatePageNumber(number),
            objects: this.get('objects').slice(beginIndex, endIndex)
        })
    },

    pageCount: function () {
        if (this.get('objectCount') === 0 && !this.get('allowEmptyFirstPage')) {
            return 0;
        }
        else {
            let hits = Math.max(1, this.get('objectCount') - this.get('orphans'));
            return Math.ceil(hits / this.get('perPage'));
        }
    }.property('perPage', 'objectCount', 'allowEmptyFirstPage', 'orphans'),

    // SimplePaginator implements Ember.Array (by PAGES, not objects) (also, 0-indexed necessary here) 
    length: Ember.computed.alias('pageCount'),
    objectAt: function (pageNumber) {
        return this.page(pageNumber - 1);
    }
});

export default Ember.Component.extend({
    // Content to page, expected to be ember-cli-pagination/PagedArray
    pagedContent: null,
    perPage: 10,
    orphans: 0,
    objects: [],
    allowEmptyFirstPage: true,

    paginator: function() {
        return SimplePaginator.create({parent: this})
    }.property(),

    actions: {
        // These actions should propagate up to the router for when more data need to be loaded.

        next: function () {

        },
        prev: function () {

        },
        jump: function (pageNumber) {

        }
    }
});

pods/better-page-numbers/template.hbs

<div class="pagination-centered">
    <ul class="pagination">
        <li class="arrow prev {{if paginator.hasPrev 'enabled-arrow' 'disabled'}} ">
            <a href="#">&laquo;</a>
        </li>

        {{!-- fill in here --}}

        <li class="arrow next {{if paginator.hasNext 'enabled-arrow' 'disabled'}} ">
            <a href="#">&raquo;</a>
        </li>
    </ul>
</div>

Hope this helps someone out.

jcope2013 commented 9 years ago

i fixed the binding issue by removing the page/perPage bindings and instead adding a action that is already internally triggered by the page-numbers component when you change a page that updates the properties on the controller, this also benefits from more of a DDAU approach which is more of the Ember way

export default Ember.Controller.extend({
  // setup our query params
  queryParams: ["page", "perPage"],

  totalPagesBinding: "content.totalPages",

  // set default values, can cause problems if left out
  // if value matches default, it won't display in the URL
  page: 1,
  perPage: 10,

  actions: {
     pageClicked(page) {
         this.setProperties({ page: page });
      }
   }
});

{{page-numbers content=pagedContent action=(action 'pageClicked')}}
ianwalter commented 8 years ago

@jcope2013's fix worked for me although I don't think perPage should be in the pageClicked function. Thanks for everyones work on this.

thejchap commented 8 years ago

@jcope2013's fix worked for me as well

DEBUG: Ember             : 2.2.0
DEBUG: Ember Data        : 2.3.1
yanatan16 commented 8 years ago

:+1: @jcope2013's fix worked for me.

DEBUG: Ember             : 2.3.0
DEBUG: Ember Data        : 2.3.3
esbanarango commented 8 years ago

@jcope2013 Thanks! forked for me as well.

jme783 commented 8 years ago

+1 @jcope2013 thank you! Seems like there should be an official fix for this though

lbblock2 commented 8 years ago

Any solutions for the remote paginated API?

remino commented 8 years ago

Any solutions for the remote paginated API?

Also doesn't work for me. Getting this error message in my console:

ember.debug.js:28556 Error while processing route: [redacted controller name] delegate is not a function TypeError: delegate is not a function
shasiuk1 commented 8 years ago

So, has anyone found a proper fix for this issue (remote paginated api) ?!

lancedikson commented 7 years ago

@jcope2013's solution works for remote paginated api as well (Ember 2.10.0 and Ember Data 2.11.0)

typeoneerror commented 7 years ago

@jcope2013's solution works but doesn't update the param in the URL for me. If I try to do what the documentation shows with page: alias('content.page'), I get

Property set failed: object in path "content" could not be found or was destroyed. Error: Property set failed: object in path "content" could not be found or was destroyed.

There seems to be a lot of conflicting setup information in docs, tests and issues. Does anyone have a definitive guide to making all the pieces of this addon work?

typeoneerror commented 7 years ago

Of note: https://github.com/emberjs/ember.js/issues/10608#issuecomment-78298761

QP's cannot be CP's, so the example in the documentation will not work. It looks like if I have

queryParams: {
  page: { refreshModel: true }
}

this is the only way to get the page URL param to change. Is this what everyone is doing?

typeoneerror commented 7 years ago

Extending @jcope2013's solution, here's what's currently working for me:

setPage(page) {
  Ember.Logger.log('setPage', page);
  this.setProperties({
    'model.page': page,
    page: page
  })
}

Setting model.page triggers the fetch of the paged-remote-array without reloading the model. Setting page triggers the URL param to change. I'm not able to just set one of the two with a computed property in any way.

sdhull commented 6 years ago

A few things here.

First, this addon as documented in the Readme will not work (with recent versions of ember / ember-data).

Second, big thanks to @typeoneerror for a good workaround and for finding out that [Query Params] cannot be [Computed Properties].

Lastly, it's worth noting that using content is deprecated and all the references to it should be updated.

typeoneerror commented 6 years ago

@sdhull 👍

I'm still on Ember 2.4 so this is working fine for me now, but I plan on migrating over to https://github.com/offirgolan/ember-light-table when I get some time to upgrade Ember.

sdhull commented 6 years ago

Another problem I've found is that page isn't reset when you navigate away and then navigate back (because controllers aren't necessarily torn down when you navigate away).

broerse commented 6 years ago

@sdhull I upgraded to 2.17 and it stopped working. Will try to take a look what changed.