Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to 2.0.x causes error #146

Open
typeoneerror opened this issue Oct 31, 2015 · 36 comments
Open

Upgrade to 2.0.x causes error #146

typeoneerror opened this issue Oct 31, 2015 · 36 comments

Comments

@typeoneerror
Copy link

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:

emberjs/ember.js#11592

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

@DylanLukes
Copy link

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

@broerse
Copy link
Collaborator

broerse commented Nov 25, 2015

@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
Copy link

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
Copy link

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
Copy link
Contributor

👍

@DylanLukes
Copy link

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

@mharris717
Copy link
Owner

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

@jcope2013 @DylanLukes @broerse

@DylanLukes
Copy link

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
Copy link
Owner

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

Thanks for all your work already.

@DylanLukes
Copy link

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
Copy link
Owner

@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
Copy link

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.

@mharris717
Copy link
Owner

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
Copy link

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

@DylanLukes
Copy link

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
Copy link
Contributor

@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
Copy link

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
Copy link

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:

emberjs/data#2905
emberjs/data#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
Copy link

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
Copy link
Contributor

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
Copy link

@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
Copy link
Contributor

@jcope2013's fix worked for me as well

DEBUG: Ember             : 2.2.0
DEBUG: Ember Data        : 2.3.1

@yanatan16
Copy link

👍 @jcope2013's fix worked for me.

DEBUG: Ember             : 2.3.0
DEBUG: Ember Data        : 2.3.3

@esbanarango
Copy link

@jcope2013 Thanks! forked for me as well.

@jme783
Copy link

jme783 commented Mar 4, 2016

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

@lbblock2
Copy link

Any solutions for the remote paginated API?

@remino
Copy link

remino commented Oct 11, 2016

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
Copy link

shasiuk1 commented Nov 5, 2016

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

@lancedikson
Copy link
Contributor

lancedikson commented Jan 17, 2017

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

@typeoneerror
Copy link
Author

@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
Copy link
Author

Of note: emberjs/ember.js#10608 (comment)

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
Copy link
Author

typeoneerror commented Feb 10, 2017

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
Copy link

sdhull commented Dec 7, 2017

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
Copy link
Author

@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
Copy link

sdhull commented Dec 7, 2017

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
Copy link
Collaborator

broerse commented Dec 7, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests