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

extract model.pushObjects to a method #54

Merged
merged 1 commit into from
Jul 17, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,19 @@ and ember-infinity will be set up to parse the total number of pages from a JSON
}
```

You may override `updateInfinityModel` to customize how the route's `model` should be updated with new objects. You may also invoke this method directly to manually push new objects into the model:

```js
actions: {
pushHughIntoInfinityModel() [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice name choice :)

var updatedInfinityModel = this.updateInfinityModel([
{ id: 1, name: "Hugh Francis" }
]);
console.log(updatedInfinityModel);
}
}
```

### infinityModel

You can also provide additional parameters to `infinityModel` that
Expand Down
20 changes: 16 additions & 4 deletions addon/mixins/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -173,7 +173,6 @@ export default Ember.Mixin.create({
var nextPage = this.get('_currentPage') + 1;
var perPage = this.get('_perPage');
var totalPages = this.get('_totalPages');
var model = this.get(this.get('_modelPath'));
var modelName = this.get('_infinityModelName');

if (!this.get('_loadingMore') && this.get('_canLoadMore')) {
Expand All @@ -187,14 +186,14 @@ export default Ember.Mixin.create({
var promise = this.store.find(modelName, params);

promise.then(
infinityModel => {
model.pushObjects(infinityModel.get('content'));
newObjects => {
this.updateInfinityModel(newObjects);
this.set('_loadingMore', false);
this.set('_currentPage', nextPage);
Ember.run.scheduleOnce('afterRender', this, 'infinityModelUpdated', {
lastPageLoaded: nextPage,
totalPages: totalPages,
newObjects: infinityModel
newObjects: newObjects
});
if (!this.get('_canLoadMore')) {
this.set(this.get('_modelPath') + '.reachedInfinity', true);
Expand All @@ -217,6 +216,19 @@ export default Ember.Mixin.create({
return false;
},

/**
Update the infinity model with new objects

@method updateInfinityModel
@param {Ember.Enumerable} newObjects The new objects to add to the model
@return {Ember.Array} returns the updated infinity model
*/
updateInfinityModel(newObjects) {
var infinityModel = this.get(this.get('_modelPath'));

return infinityModel.pushObjects(newObjects.get('content'));
},

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still finding this confusing. the wording infinityModel should always refer to the route's model, ie, the model stored at _modelPath.

I think we should do

updateInfinityModel(collection, infinityModel) {
    if (!infinityModel) {
      infinityModel = this.get('_modelPath');
    }
    return infinityModel.pushObjects(collection.get('content'));
}

This would imply we'd want to change the server response in the promise above from infinityModel to collection, and then line 191 to: this.updateInfinityModel(collection, model);

I think the wording infinityModel should canonically always refer to the object at route.get('_modelPath') and nothing else.

This also means that developers can also use this function like so:

actions: {
  pushHughIntoInfinityModel() [
    var updatedInfinityModel = this.updateInfinityModel([
      { id: 1, name: "Hugh Francis" }
    ]);
    console.log(updatedInfinityModel);
  }
}

...which is something some other devs have asked about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍, I really like that. Will make that change and potentially update the README too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Psyched! sorry for the nitpicks, just think this is an important addition to the API and I wanna get it right xo

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhh now the problem is that we've pulled the newObjects.get('content') back into the load method. That means, if someone is using pure arrays without prototype extensions, there won't be a good place for them to override the get('content') part of the push.

I like this idea because it exposes the method for actually extracting the data from the collection that is pushed into the infinityModel, giving developers a public place to change that logic.

Can we have updateInfinityModel do the get('content') part of the extraction for easy override?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, but doesn't that mean you would no longer be able to do updateInfinityModel with a literal array?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the example you gave wouldn't be possible if we did the get('content') inside this method (without an override).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would work for the majority of developers, unless they've explicitly turned off prototype extensions, and if they have, they're probably going to be aware of that stuff. They can simply override the method in that case and do a switch on whether it's a literal array of a model collection.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good. Will make the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, apologies for the delay.

actions: {
infinityLoad() {
this._infinityLoad();
Expand Down
67 changes: 67 additions & 0 deletions tests/unit/mixins/route-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -308,3 +308,70 @@ test('it uses overridden params when loading more data', assert => {
assert.ok(model.get('reachedInfinity'), 'Should reach infinity');

});

test('it allows overrides/manual invocations of updateInfinityModel', assert => {
var RouteObject = Ember.Route.extend(RouteMixin, {
model() {
return this.infinityModel('item', {perPage: 1});
},
updateInfinityModel(newObjects) {
return this._super(newObjects.setEach('author', 'F. Scott Fitzgerald'));
}
});
var route = RouteObject.create();

var items = [
{ id: 1, title: 'The Great Gatsby' },
{ id: 2, title: 'The Last Tycoon' }
];

var dummyStore = {
find(modelType, findQuery) {
var item = items[findQuery.page-1];
return new Ember.RSVP.Promise(resolve => {
Ember.run(this, resolve, Ember.ArrayProxy.create({
content: Ember.A([item]),
meta: { total_pages: 2 }
}));
});
}
};

route.store = dummyStore;

var model;
Ember.run(() => {
route.model().then(result => {
model = result;
});
});

var dummyController = Ember.Object.create({
model
});
route.set('controller', dummyController);

assert.equal(route.get('_canLoadMore'), true);
assert.equal(model.get('content.length'), 1);

Ember.run(() => {
route._infinityLoad();
});

assert.equal(route.get('_canLoadMore'), false);
assert.equal(model.get('content.length'), 2);
assert.equal(model.get('content.lastObject.author'), 'F. Scott Fitzgerald', 'overrides to updateInfinityModel should take effect');

var newObjects = Ember.ArrayProxy.create({
content: Ember.A([
{ id: 3, title: 'Tender Is the Night' }
])
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a little bit laborious to construct... I might be wrong, but I don't think manual invocations of updateInfinityModel as specified in your example @hhff will work even with prototype extensions. However, it's still at least possible, and maybe it's nice to have the flexibility of being able to access meta parameters in updateInfinityModel.

So my default plan will be to keep this and update the README so that instead of

var updatedInfinityModel = this.updateInfinityModel([
  { id: 1, name: "Hugh Francis" }
]);

we have:

var updatedInfinityModel = this.updateInfinityModel(Ember.ArrayProxy.create({
  content: Ember.A([
    { id: 1, name: "Hugh Francis" }
  ])
});

Let me know how that sounds or if I just am misunderstanding prototype extensions!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, reading up on it a bit more, I was definitely misunderstanding prototype extensions and some aspects of this addon. Prototype extensions are disabled locally in this project by the inclusion of ember-disable-prototype-extensions in the addon's devDependencies, because addons should be maximally compatible, but we expect that most consumers of the addon will have them enabled. So hopefully the example above should work?

The only thing I'm not sure about now is whether an ES6 or ES5 prototype-extended Array will respond properly to .get('content'). But I think I don't need to update the README as drastically as I thought. This has been a nice learning experience.

tl;dr: this is probably ready now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah! I think this is ready...! I think if people want to use non-prototype extension'd collections, we'll just recommend they override the hook to account for that case.


Ember.run(() => {
route.updateInfinityModel(newObjects);
});

assert.equal(model.get('content.length'), 3);
assert.equal(model.get('content.lastObject.title'), 'Tender Is the Night', 'updateInfinityModel can be invoked manually');
});