-
Notifications
You must be signed in to change notification settings - Fork 116
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
Comments
+1, will update with more as I investigate this. |
@mharris717 and I like to update to 2.0 soon. @DylanLukes if you switch to 2.0 are there errors in |
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:
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);
}, |
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:
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. |
👍 |
Currently rewriting a new |
Anyone have interest in pairing with me on this? I apologize for being a shit maintainer. |
Unfortunately, I can't currently take up maintaining anything.
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 just looking for somebody to work with me on a single pairing session. Thanks for all your work already. |
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 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. |
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)
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. |
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. |
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 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 |
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 why is the
needed? or what is the intention of that? I am sort of confused there |
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 |
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 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 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. |
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 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.
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) {
}
}
});
<div class="pagination-centered">
<ul class="pagination">
<li class="arrow prev {{if paginator.hasPrev 'enabled-arrow' 'disabled'}} ">
<a href="#">«</a>
</li>
{{!-- fill in here --}}
<li class="arrow next {{if paginator.hasNext 'enabled-arrow' 'disabled'}} ">
<a href="#">»</a>
</li>
</ul>
</div> Hope this helps someone out. |
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
|
@jcope2013's fix worked for me although I don't think |
@jcope2013's fix worked for me as well
|
👍 @jcope2013's fix worked for me.
|
@jcope2013 Thanks! forked for me as well. |
+1 @jcope2013 thank you! Seems like there should be an official fix for this though |
Any solutions for the remote paginated API? |
Also doesn't work for me. Getting this error message in my console:
|
So, has anyone found a proper fix for this issue (remote paginated api) ?! |
@jcope2013's solution works for remote paginated api as well (Ember 2.10.0 and Ember Data 2.11.0) |
@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
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? |
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
this is the only way to get the page URL param to change. Is this what everyone is doing? |
Extending @jcope2013's solution, here's what's currently working for me:
Setting |
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 Lastly, it's worth noting that using |
@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. |
Another problem I've found is that |
@sdhull I upgraded to 2.17 and it stopped working. Will try to take a look what changed. |
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.
The text was updated successfully, but these errors were encountered: