Call update model on first page load

[davidgoli]

Fixes a bug in ember-infinity. See: #90

In short, there was no effective hook to update boundParams after the first page loads, but before the second page is requested. This PR fixes that.

[hhff]

Weird! Looks good to me. @mike-north / @kellyselden / @mkorfmann can i get a second set of eyes?

[ghost]

It's sufficient to check for infinityModel being undefined here (or you need to add another test-case which explains the checking of newObjects).

[ghost]

Merging this PR would definitely resonate with me since the overwriting of 'updateInfinityModel' is encouraged in the README and it should work as expected for the first batch of records as well.

But introducing a (or fixing a existing) hook which has no other side effects except of, well, being a hook, would be even better IMO.

So while this fix definitely makes things work more as expected right now, I'm not sure whether encouraging patching of 'updateInfinityModel' is the ideal API for making changes to records after loading.

[davidgoli]

How about introducing a new "first page" hook like afterInfinityModel or
something? I do think from an ideal API perspective, it is strange to have
one hook for the first page and a different one for 2+. Perhaps the default
implementation could call updateInfinityModel and clients that need to can
override the extra hook, but the default behavior would be more intuitive.
On Sep 15, 2015 9:45 PM, "Manuel Arno Korfmann" notifications@github.com
wrote:

Merging this PR would definitely resonate with me since the overwriting of
'updateInfinityModel' is encouraged in the README and it should work as
expected for the first batch of records as well.

But introducing a (or fixing a existing) hook which has no other side
effects except of, well, being a hook, would be even better IMO.

So while this fix definitely makes things work more as expected right now,
I'm not sure whether encouraging patching of 'updateInfinityModel' is the
ideal API for making changes to records after loading.

—
Reply to this email directly or view it on GitHub
#89 (comment).

[ghost]

I'm not sure if introducing a new hook is the right way either, maybe I should've put that more clearly.

For me it feels like we only should need one hook to modify the records (or to do something else) after the model was loaded or am I missing something completely here?

[ghost]

And 'updateInfinityModel' looks like it should be private API to me since it's needed after the initial batch was loaded.

[davidgoli]

What would be really sweet IMO is if you could observe the infinityModel
with your own computed properties that are in turn used as bound params.
For instance,

minId: Ember.computed('infinityModel.[]', function () {
  return this.get('infinityModel.lastObject.id');
})

The change to the infinity model's array would happen in the same run loop
as the model is updated to ensure that changes are picked up before the
next request is made.

[ghost]

Definitely a cool idea, but out of the scope of this PR I guess.

So just for the record: I'd vote for merging this PR (just some minor nitpicking), since it makes overwriting updateInfinityModel work as expected.

After merging we should think about a more streamlined approach to the whole hook subject, but this is also out of the scope of this PR.

[hhff]

@davidgoli - thoughts here?

[davidgoli]

@hhff this PR fixes the bug, and also fulfills the promise of the documentation, which currently says: "You may override updateInfinityModel to customize how the route's model should be updated with new objects." But updateInfinityModel does not work in this way, as it's not called on the first page.

However, in a different PR, you raised an objection re: users who may be relying on the buggy behavior. Additionally, with this PR, users who override updateInfinityModel will have to call return this._super(newObjects); as the last line, or else it won't work as expected. Here I'm looking to Ember Core for inspiration; most of their hooks are merely events, and don't require a call to _super. On the other hand, there are hooks like setupController that provide a superclass implementation that you can choose to call or not to call depending on the needs of your application. So I suppose requiring a call to _super - especially given how current users already have to call _super - is not the worst thing.

I think this PR is GTG. I have also left comments on #77, which I view as an acceptable alternative after some changes. But, I would want one or the other - and if #77 gets merged, I'd propose deprecating updateInfinityModel for future versions since it's buggy and also is a nearly duplicate hook to afterInfinityModel from that branch.

[hhff]

@davidgoli - I think we're going to go with #77 in favor of this - and deprecate updateInfinityModel as you suggest ❤️

Are you interested in making the deprecation change (we'll want to start using Ember.warn or something similar for #96 too)?

I'm on holiday right now (hence the slow replies) 🐚

[hhff]

closing this in favor of #105