-
-
Notifications
You must be signed in to change notification settings - Fork 131
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
Conversation
@@ -217,6 +217,10 @@ export default Ember.Mixin.create({ | |||
return false; | |||
}, | |||
|
|||
_updateModel(model, infinityModel) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the purpose is overriding, wouldn't you want to make it public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
I would suggest a README change to help others with this. Then squash commits and LGTM. |
@@ -217,6 +217,10 @@ export default Ember.Mixin.create({ | |||
return false; | |||
}, | |||
|
|||
updateModel(model, infinityModel) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hhff I think the point of the infinityModel
param is to use it when overriding without accessing anything private.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its fine to use private methods inside a public method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that when overriding, people should use _modelPath
to get the infinityModel
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, im saying we don't even need the 2nd arg, cause we can retrieve it from the route
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I think you know better than me in this area, I'm going to bow out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it to updateInfinityModel
to avoid name collision, hope that's alright and sorry for inadvertently hiding this chain of comments.
I updated the method signature and also renamed some variables so that Will squash all the commits if this looks good. |
|
||
return infinityModel.pushObjects(newObjects); | ||
}, | ||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
this is perfect @asross - can you put up a simple test? Once we've got some test coverage this is good to merge |
|
||
```js | ||
actions: { | ||
pushHughIntoInfinityModel() [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice name choice :)
Can u give it a quick squash? |
to allow overrides in the case when more specialized munging/manipulation might be required
Yep, done! |
extract `model.pushObjects` to a method
👍 |
Nice work. |
Thanks! |
...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.