extract `model.pushObjects` to a method #54

asross · 2015-07-08T02:31:10Z

...to allow overrides in the case when more specialized munging/manipulation might be required.

For one part of our app that needs infinite scroll, we have a slightly non-traditional API that requires a more complicated merging of our initial data and subsequent requests. This extremely minor change makes it much easier for us to use ember-infinity in that case.

kellyselden · 2015-07-08T02:48:24Z

addon/mixins/route.js

@@ -217,6 +217,10 @@ export default Ember.Mixin.create({
    return false;
  },

+  _updateModel(model, infinityModel) {


If the purpose is overriding, wouldn't you want to make it public?

Yeah agree! I was under the impression that the "pushObjects" to model content no longer works in new versions of Ember Data. What version of Ember Data are you using?

Agreed about publicity, will un-underscore the method name. I'm not sure about the compatibility of pushObjects with the latest Ember data, but that doesn't seem relevant to this change (since I'm just moving code around).

kellyselden · 2015-07-08T13:02:20Z

I would suggest a README change to help others with this. Then squash commits and LGTM.

hhff · 2015-07-08T13:53:37Z

addon/mixins/route.js

@@ -217,6 +217,10 @@ export default Ember.Mixin.create({
    return false;
  },

+  updateModel(model, infinityModel) {


We also don't need to pass infinityModel here, we can get it with this.get('_modelPath').

Can we also rename this method to addObjects, and the first arg to objects? Want to be explicit that we aren't updating or changing the infinityModel in place, we're just adding to it.

Also - a small note in the README would be great!

@hhff I think the point of the infinityModel param is to use it when overriding without accessing anything private.

Its fine to use private methods inside a public method?

Are you saying that when overriding, people should use _modelPath to get the infinityModel?

no, im saying we don't even need the 2nd arg, cause we can retrieve it from the route

Hmm. I think you know better than me in this area, I'm going to bow out.

I may be missing something! The purpose of this method is just for conveniently pushing arbitary objects into the infinityModel from the route, right @asross ?

Sorry for being so late to reply! I'd be in favor of keeping infinityModel as a parameter because we don't necessarily want users of this addon to have to think deeply about implementation details of ember-infinity, instead we just want them to specify how the response from the server (infinityModel) should update the route's data (model). So from an addon-overrider user experience point of view, I think it's slightly nicer to have it be an argument.

Another argument-argument is that you might change the private variable name later, but you don't necessarily want overriders of the addon to have to change their code.

But, from a general software point of view, it does make more sense to omit the second argument since it is superfluous. And if you're overriding this method, even though it's public, you maybe know what you're in for.

In any case, I'm happy with either, the main thing is to have a separate method. I'll add something to the README to this effect and whichever you think is better @hhff we will go with.

Actually, I'm wondering if we should rename this method to something more specific to ember-infinity? It's conceivable that an app could already have a route method called updateModel that's unrelated to this addon.

I changed it to updateInfinityModel to avoid name collision, hope that's alright and sorry for inadvertently hiding this chain of comments.

asross · 2015-07-09T14:45:06Z

I updated the method signature and also renamed some variables so that infinityModel consistently refers to the route's model rather than the new objects to be added to it. Also did end up removing the model parameter, because whoever is overriding this function knows best how to access the route's model. Let me know if the README example which I c/ped from you is ok 😁

Will squash all the commits if this looks good.

hhff · 2015-07-10T11:51:43Z

addon/mixins/route.js

+
+    return infinityModel.pushObjects(newObjects);
+  },
+


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?

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

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

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.

Ok, sounds good. Will make the change.

Done, apologies for the delay.

hhff · 2015-07-14T17:37:27Z

this is perfect @asross - can you put up a simple test? Once we've got some test coverage this is good to merge

kellyselden · 2015-07-14T17:45:48Z

README.md

+
+```js
+actions: {
+  pushHughIntoInfinityModel() [


nice name choice :)

hhff · 2015-07-17T09:45:02Z

Can u give it a quick squash?

to allow overrides in the case when more specialized munging/manipulation might be required

asross · 2015-07-17T12:15:36Z

Can u give it a quick squash?

Yep, done!

extract `model.pushObjects` to a method

hhff · 2015-07-17T13:49:58Z

👍

kellyselden · 2015-07-17T13:50:58Z

Nice work.

asross · 2015-07-17T15:41:51Z

Thanks!

kellyselden reviewed Jul 8, 2015
View reviewed changes

hhff reviewed Jul 8, 2015
View reviewed changes

hhff reviewed Jul 10, 2015
View reviewed changes

hhff mentioned this pull request Jul 11, 2015

deleteOld #56

Closed

asross force-pushed the master branch from 53dc25c to b7cd72f Compare July 13, 2015 17:04

kellyselden reviewed Jul 14, 2015
View reviewed changes

README.md

```js

actions: {

pushHughIntoInfinityModel() [

Copy link

Collaborator

kellyselden Jul 14, 2015

Choose a reason for hiding this comment

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

nice name choice :)

extract model.pushObjects to a method

89efdec

to allow overrides in the case when more specialized munging/manipulation might be required

asross force-pushed the master branch from fdda0b1 to 89efdec Compare July 17, 2015 12:15

hhff added a commit that referenced this pull request Jul 17, 2015

Merge pull request #54 from asross/master

a61a7f3

extract `model.pushObjects` to a method

hhff merged commit a61a7f3 into adopted-ember-addons:master Jul 17, 2015

franklington mentioned this pull request Aug 19, 2015

this.refresh() unbinds Eventhandler #78

Closed

snyk-bot mentioned this pull request Aug 24, 2020

[Snyk] Upgrade ember-cli-htmlbars from 0.7.6 to 0.7.10 rsig/ember-infinity#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

extract `model.pushObjects` to a method #54

extract `model.pushObjects` to a method #54

asross commented Jul 8, 2015

kellyselden Jul 8, 2015

hhff Jul 8, 2015

asross Jul 8, 2015

kellyselden commented Jul 8, 2015

hhff Jul 8, 2015

kellyselden Jul 8, 2015

hhff Jul 8, 2015

kellyselden Jul 8, 2015

hhff Jul 8, 2015

kellyselden Jul 8, 2015

hhff Jul 8, 2015

asross Jul 9, 2015

asross Jul 9, 2015

asross Jul 9, 2015

asross commented Jul 9, 2015

hhff Jul 10, 2015

asross Jul 10, 2015

asross Jul 10, 2015

hhff Jul 10, 2015

asross Jul 10, 2015

asross Jul 13, 2015

hhff commented Jul 14, 2015

kellyselden Jul 14, 2015

hhff commented Jul 17, 2015

asross commented Jul 17, 2015

hhff commented Jul 17, 2015

kellyselden commented Jul 17, 2015

asross commented Jul 17, 2015

extract model.pushObjects to a method #54

extract model.pushObjects to a method #54

Conversation

asross commented Jul 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyselden commented Jul 8, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asross commented Jul 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhff commented Jul 14, 2015

Choose a reason for hiding this comment

hhff commented Jul 17, 2015

asross commented Jul 17, 2015

hhff commented Jul 17, 2015

kellyselden commented Jul 17, 2015

asross commented Jul 17, 2015

extract `model.pushObjects` to a method #54

extract `model.pushObjects` to a method #54