ember-cli-pagination icon indicating copy to clipboard operation
ember-cli-pagination copied to clipboard

Upgrade to 2.0.x causes error

Open typeoneerror opened this issue 9 years ago • 36 comments

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.

typeoneerror avatar Oct 31 '15 00:10 typeoneerror

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

DylanLukes avatar Nov 25 '15 16:11 DylanLukes

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

broerse avatar Nov 25 '15 18:11 broerse

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 avatar Nov 25 '15 18:11 DylanLukes

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.

DylanLukes avatar Nov 25 '15 19:11 DylanLukes

:+1:

jcope2013 avatar Nov 25 '15 19:11 jcope2013

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

DylanLukes avatar Nov 25 '15 20:11 DylanLukes

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

@jcope2013 @DylanLukes @broerse

mharris717 avatar Nov 25 '15 20:11 mharris717

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.

DylanLukes avatar Nov 25 '15 21:11 DylanLukes

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

Thanks for all your work already.

mharris717 avatar Nov 25 '15 21:11 mharris717

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".

DylanLukes avatar Nov 25 '15 21:11 DylanLukes

@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.

mharris717 avatar Nov 25 '15 21:11 mharris717

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)

  • currently this is propagated onto the PagedArray, not the controller, which necessitates the bindings. In Ember 2.0 we can't sanely bind these from the model to the controller, we have to rely on the router to set the properties on the controller/model and handle other state needs with property assignment.

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.

DylanLukes avatar Nov 25 '15 21:11 DylanLukes

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.

mharris717 avatar Nov 25 '15 21:11 mharris717

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:

  • Router fetches data using current pagination query parameters.
  • The data ([myModel]) is split from the metadata (currentPage, totalPages, ...)
  • The actual entities are passed as a standard Ember.Array to a component, with the metadata passed separately if desired.

(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 avatar Nov 25 '15 21:11 DylanLukes

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.

DylanLukes avatar Nov 25 '15 21:11 DylanLukes

@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

jcope2013 avatar Nov 25 '15 22:11 jcope2013

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 avatar Nov 25 '15 23:11 DylanLukes

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 avatar Nov 26 '15 15:11 DylanLukes

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.

DylanLukes avatar Dec 01 '15 08:12 DylanLukes

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')}}

jcope2013 avatar Dec 04 '15 01:12 jcope2013

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

ianwalter avatar Dec 12 '15 03:12 ianwalter

@jcope2013's fix worked for me as well

DEBUG: Ember             : 2.2.0
DEBUG: Ember Data        : 2.3.1

thejchap avatar Jan 18 '16 18:01 thejchap

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

DEBUG: Ember             : 2.3.0
DEBUG: Ember Data        : 2.3.3

yanatan16 avatar Jan 28 '16 22:01 yanatan16

@jcope2013 Thanks! forked for me as well.

esbanarango avatar Feb 29 '16 20:02 esbanarango

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

jme783 avatar Mar 04 '16 01:03 jme783

Any solutions for the remote paginated API?

lbblock2 avatar Jul 27 '16 23:07 lbblock2

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

remino avatar Oct 11 '16 07:10 remino

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

shasiuk1 avatar Nov 05 '16 20:11 shasiuk1

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

lancedikson avatar Jan 17 '17 13:01 lancedikson

@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 avatar Feb 09 '17 23:02 typeoneerror