Add afterInfinityModel #77

kxcrl · 2015-08-18T19:33:29Z

In the application that I'm working on, there are many cases where get(this, 'store').find(modelName, params) is not the end of the promise chain before we hand the fetched models to the route. This PR adds that functionality.

I tried to follow the conventions that I could see for declaring private parameters, using the options object, and deleting keys after creating variables for them. That said, it does seem a little odd to have an _afterModelPromise private property, and you might have better suggestions for how things should be named.

kxcrl · 2015-08-18T20:43:27Z

Sorry about opening a PR with test failures. Before I go in and fix them up, I wanted to make sure that this was actually an option you were interested in and an approach that makes sense. If it is, I'm happy to go in and update all of the testing. If the approach doesn't make sense, I'm also happy to try something else.

hhff · 2015-08-19T09:36:46Z

innnnnneresting @kxcrl ! thanks for this. Let me mull it over a couple days while we work on supporting ED 1.13x

kxcrl · 2015-08-19T15:38:28Z

@hhff Cool. = )

Sounds good. Again, just let me know if you'd like to see any of it changed or rewritten.

mike-north · 2015-09-07T01:02:46Z

👍 to this idea. Thanks for the PR!

hhff · 2015-09-07T10:05:34Z

I think I'd like the ergonomics to feel more like the regular afterModel() method on the route. Something like

model() {
  return this.infinityModel('product',  { perPage: 12, startingPage: 1 });
},
afterInfinityModel(infinityModelPromise) {
  return infinityModelPromise.then(collection => {
    let preloadPromises = [];

    collection.forEach(item => {
      preloadPromises.push(item.get('somethingAsync'));
    });

    return Ember.RSVP.all(preloadPromises);
  });
}

default would look like:

afterInfinityModel(infinityModelPromise) {
  return infinityModelPromise;
}

I'm a little worried that this may obscure the semantics of how the model hook works though - ie: where should a "catch" live?

@kxcrl / @mike-north / @kellyselden / @mkorfmann thoughts?

kxcrl · 2015-09-09T17:46:44Z

@hhff I think that sounds great. I'm not sure I fully follow your concern, though - can you expound on it a bit more? Is it just about the extra overhead in making sure that any promise passed into the chain has a way to fail gracefully?

hhff · 2015-09-10T15:03:50Z

@kxcrl I just mean that exposes a couple of places where developers can handle failed promises, instead of just one obvious place. I don't think it's a big concern tho - also, now you can catch failures in the infinityLoad action promise style - which is a new feature.

Do you want to have a pass at implementing it in this style brother?

kxcrl · 2015-09-10T15:48:05Z

@hhff Ah, gotcha. Yep, will do!

kxcrl · 2015-09-18T19:05:33Z

@hhff Alright, I finally had some time to sit down and implement this. Maybe I did something wrong, but your version was very easy to get working. Let me know what you think and I'll get it wrapped in some tests.

hhff · 2015-09-19T17:28:03Z

addon/mixins/route.js

@@ -174,7 +174,8 @@ export default Ember.Mixin.create({
    }

    var params = Ember.merge(requestPayloadBase, options);
-    let promise = this.store[this._storeFindMethod](modelName, params);
+
+    let promise = this.afterInfinityModel(this.store[this._storeFindMethod](modelName, params));


I think afterInfinityModel should be called on the resolved outcome of the promise - that way you know the object passed in is data, rather than a thennable.

This is closer to how the regular afterModel works

Ha, there was another implementation of this that I tried out and didn't commit:

let promise = this.store[this._storeFindMethod](modelName, params).then(afterInfinityModel);

Something like that?

yuuuuuuuuuuuuup

hhff · 2015-09-19T17:28:36Z

👍 awesome work - added a comment in!

kxcrl · 2015-09-22T21:56:34Z

Aaaaand maybe that'll do it? I updated the afterInfinityModel method and went ahead and added a test for it.

kxcrl · 2015-10-01T16:53:45Z

@hhff Just checking in real quick. Anything else you'd like to see on this one?

hhff · 2015-10-02T11:12:11Z

tests/unit/mixins/route-test.js

+      return this.infinityModel('item');
+    },
+    afterInfinityModel(items) {
+      return items.setEach('author', 'F. Scott Fitzgerald');


could we also do a test that uses a second promise as the return?

something like:

afterInfinityModel(items) { return new Promise((resolve) => { resolve(items.setEach('author', 'F. Scott Fitzgerald')); }); }

hhff · 2015-10-02T11:13:13Z

thanks so much @kxcrl !

Could u add a test for a returning promise (comment above)

A Readme Note about this functionality
Merge master & squash

and then we're good!! You're the man thankyou!

kxcrl · 2015-10-06T19:15:33Z

@hhff Awesome! Thanks for all the feedback and walking me through everything you were looking for. Let me know if there's anything else that needs changing.

hhff · 2015-10-07T11:09:03Z

Amazing...! I'm very excited for this. Any idea why beta is failing on travis?

kxcrl · 2015-10-07T15:42:45Z

@hhff Great question. I read through the 2.2.0-beta.1 changelog and couldn't find anything that would seem to cause this. I can reproduce the failure locally with ember try ember-beta test, but unfortunately it's still there when I do this on the current master branch without my changes, too.

If you checkout df5e340b64334db9d2d95caae37d3d7e3b3ad279, the error goes away. As soon as you add in c7ccf4be414af3d0b632e219e98be84a115bcd0d, the error appears, so it looks like it got introduced with something that was changed in #72.

I tried a few things with the failing test itself, but I ran out of time before I could get anywhere.

hhff · 2015-10-08T11:31:54Z

Thanks @kxcrl - do you have some time to play around with those changes made in #72 and figure out what is causing it? Nothing in that PR seems fishy...

kxcrl · 2015-10-08T16:29:56Z

@hhff Hmmm, possibly, but unfortunately not until later next week at the earliest. I'm also wondering if it might just be a bug in the latest ember beta. Is this something you want fixed before you merge any pulls?

hhff · 2015-10-08T16:31:26Z

Yeah Ideally! No rush / no stress :)

davidgoli · 2015-10-16T19:45:31Z

addon/mixins/route.js

+   @return {Ember.RSVP.Promise}
+   */
+  afterInfinityModel(infinityModelPromiseResult) {
+    return infinityModelPromiseResult;


since this gets used as a chained promise, it requires the end-user code to make sure to call _super and also return the correct result needed downstream by updateInfinityModel. It would be better if it was an optional hook like so:

_afterInfinityModel(infinityModelPromiseResult) { if (typeof this.afterInfinityModel === 'function') { this.afterInfinityModel(infinityModelPromiseResult); } return infinityModelPromiseResult; },

I'm not too sure if a call to _super is needed. You're right about it expecting afterInfinityModel to return the objects, which I don't like too much either.

What's your take on this? @kxcrl

Provided the user returns something that can get('content') we should be fine yeah?

Would be ideal IMHO if the user didn't have to remember to do anything at all. They have the option to alter infinityModelPromiseResult in-place as is.

If returning something from the hook is a desired feature, that can be added, but it should be seen as a feature-add not a requirement IMO.

regardless of the ember-infinity implementation (always flowing through the hook vs only flowing through it if defined) the developer must always be sure the return implements get('content') though... right?

The difference is if we always flow through the hook, the developer can call _super (conditionally, perhaps) if they feel like it.

afterInfinityModel(newObjects) { if (mercuryInRetrograde) { return doWork(newObjects) } else { this._super.apply(...arguments); } }

I'd imagine always flowing through a function is also sliiiiiiiiightly quicker for the machine

ahhh i actually understand what you're saying now @davidgoli sorry

you're saying "do the afterInfinityModel work" in tandem to the actual return, right?

like, it could return syncronously but also trigger an async update, so it wouldn't block the population of the infinityModel yeah?

aah not exactly. There's already an infinityModelUpdated hook that executes async, and it doesn't work for #90 because the user route needs to update itself synchronously, before another request can be made.

If we go with this PR, I'd propose treating afterInfinityModel EXACTLY the same as Ember.Route#afterModel (http://emberjs.com/api/classes/Ember.Route.html#method_afterModel), if the value returned from this hook is a promise, the transition will pause until the transition resolves. Otherwise, non-promise return values are not utilized in any way.

I agree @davidgoli 👍 we should wait if it's a promise!

kxcrl · 2015-10-30T19:46:06Z

@hhff @davidgoli @ManuelArno Sorry for the long delay. I went through and rebased this and added in the changes that you all were requesting to #_afterInfinityModel. I'm not sure if I implemented them the way you were looking for. Give it a look and see what you think. If there's something I'm not understanding about it, I'd be happy to hand this off.

davidgoli · 2015-10-30T19:47:26Z

README.md

+  })
+  .then(function(posts) {
+    return posts
+  });


I think this then does nothing and can be removed?

davidgoli · 2015-10-30T19:52:36Z

addon/mixins/route.js

+        return _this.afterInfinityModel(infinityModelPromiseResult);
+      };
+    } else {
+      return infinityModelPromiseResult;


shouldn't this be wrapped in a function? in either case, the return value of this method should be a function that accepts infinityModelPromiseResult. In fact you might just do:

_afterInfinityModel(_this) { return function (infinityModelPromiseResult) { if (typeof _this.afterInfinityModel === 'function') { return _this.afterInfinityModel(infinityModelPromiseResult); } else { return infinityModelPromiseResult; } }; }

I'm not seeing a test failure for this though, but I'm p sure infinityModelPromiseResult isn't defined when the method is called on line 270

kxcrl · 2015-10-30T22:18:52Z

Right on both counts, @davidgoli

hhff · 2015-11-12T03:01:28Z

README.md

+    post.get('author')
+  })
+}
+```


We'd need a return in the afterInfinityModel function, right? Otherwise infinityModel would end up as null. (here and above)

Also, I don't think this code would actually work. We'd need an RSVP#all I think!

afterInfinityModel: function(posts) { return Ember.RSVP.all(posts.map(function(post) { post.get('author'); }); }

there also aren't enough closing parens here:

afterInfinityModel: function(posts) { return Ember.RSVP.all(posts.map(function(post) { return post.get('author'); })); }

hhff · 2015-11-12T03:03:01Z

@davidgoli / @ManuelArno - are u guys happy with this?

After the changes to the Readme - I'm happy to release this badboi

ghost · 2015-11-12T09:52:56Z

👍

2015-11-12 4:03 GMT+01:00 Hugh Francis [email protected]:

@davidgoli https://github.com/davidgoli / @ManuelArno
https://github.com/ManuelArno - are u guys happy with this?

After the changes to the Readme - I'm happy to release this badboi

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

Manuel Arno Korfmann
Luetzowstrasse 8
24105 Kiel

manu @ korfmann . info
+49 40 210913788

www.manuel-korfmann.de

davidgoli · 2015-11-12T20:16:56Z

addon/mixins/route.js

@@ -250,7 +267,7 @@ export default Ember.Mixin.create({
    const nextPage    = this.incrementProperty('currentPage');
    const params      = this._buildParams(nextPage);

-    return this.store[this._storeFindMethod](modelName, params);
+    return this.store[this._storeFindMethod](modelName, params).then(this._afterInfinityModel(this));


please wrap before .then to reduce line length

davidgoli · 2015-11-12T20:23:54Z

A few nitpicks but otherwise looking solid!

ghost · 2015-11-12T20:27:36Z

great nitpicks @davidgoli ! one can never nitpick enough with software ore one has to nitpick later (and longer) 👍

hhff · 2015-11-13T00:01:30Z

Thanks @davidgoli - I just made you a collab to this repo. When you're happy with this PR - please merge it and I'll cut a release!

davidgoli · 2015-11-17T03:43:23Z

Thanks @hhff ! @kxcrl Let me know if you're able to address the comments, and we'll merge this.

kxcrl · 2015-11-17T19:39:34Z

@davidgoli K. Things got pretty busy with work again, but I'm hoping to have time to fix this up real quick tomorrow.

davidgoli · 2015-11-20T04:42:58Z

@kxcrl any updates? would be great to get this out!

kxcrl · 2015-11-20T15:48:25Z

@davidgoli I implemented a couple of your changes. I couldn't figure out how to refactor the tests to use the new method with the time I had and Ember.typeof caused problems, so those parts aren't done. You're welcome to take this over if you want and implement those other changes if you'd like to see it finished soon and know right away what it takes.

davidgoli · 2015-11-20T19:49:16Z

@kxcrl I don't have push access to your repository, so I'm just going to check it out and create a new branch in the main repo to finish the work. Thanks so much for your help!

hhff · 2015-11-21T00:54:11Z

Closing this in favor of #105

amazing work @kxcrl & @davidgoli !

kxcrl · 2015-11-21T06:06:46Z

@davidgoli Awesome. 👍
@hhff Sounds good!

hhff reviewed Sep 19, 2015
View reviewed changes

hhff reviewed Oct 2, 2015
View reviewed changes

kxcrl force-pushed the master branch from 2f7c863 to 9c3bfe1 Compare October 6, 2015 19:11

kxcrl changed the title ~~Add afterModelPromise option~~ Add afterInfinityModel Oct 6, 2015

kxcrl force-pushed the master branch from 9c3bfe1 to 7a32029 Compare October 6, 2015 19:13

hhff mentioned this pull request Oct 13, 2015

Reduce duplication in route #88

Merged

davidgoli reviewed Oct 16, 2015
View reviewed changes

davidgoli mentioned this pull request Oct 20, 2015

Call update model on first page load #89

Closed

kxcrl force-pushed the master branch from 7a32029 to 0aee0e2 Compare October 30, 2015 19:43

davidgoli reviewed Oct 30, 2015
View reviewed changes

README.md

})

.then(function(posts) {

return posts

});

Copy link

Collaborator

davidgoli Oct 30, 2015

Choose a reason for hiding this comment

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

I think this then does nothing and can be removed?

davidgoli reviewed Oct 30, 2015
View reviewed changes

kxcrl force-pushed the master branch from 0aee0e2 to e161246 Compare October 30, 2015 22:18

hhff reviewed Nov 12, 2015
View reviewed changes

davidgoli reviewed Nov 12, 2015
View reviewed changes

davidgoli mentioned this pull request Nov 17, 2015

Cursor-based pagination loads duplicate results #90

Closed

Add afterInfinityModel

5f9b293

kxcrl force-pushed the master branch from e161246 to 5f9b293 Compare November 20, 2015 15:45

davidgoli mentioned this pull request Nov 20, 2015

Add afterInfinityModel #105

Merged

hhff closed this Nov 21, 2015

Add afterInfinityModel #77

Add afterInfinityModel #77

Conversation

kxcrl commented Aug 18, 2015

kxcrl commented Aug 18, 2015

hhff commented Aug 19, 2015

kxcrl commented Aug 19, 2015

mike-north commented Sep 7, 2015

hhff commented Sep 7, 2015

kxcrl commented Sep 9, 2015

hhff commented Sep 10, 2015

kxcrl commented Sep 10, 2015

kxcrl commented Sep 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhff commented Sep 19, 2015

kxcrl commented Sep 22, 2015

kxcrl commented Oct 1, 2015

Choose a reason for hiding this comment

hhff commented Oct 2, 2015

kxcrl commented Oct 6, 2015

hhff commented Oct 7, 2015

kxcrl commented Oct 7, 2015

hhff commented Oct 8, 2015

kxcrl commented Oct 8, 2015

hhff commented Oct 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

kxcrl commented Oct 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kxcrl commented Oct 30, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhff commented Nov 12, 2015

ghost commented Nov 12, 2015

Choose a reason for hiding this comment

davidgoli commented Nov 12, 2015

ghost commented Nov 12, 2015

hhff commented Nov 13, 2015

davidgoli commented Nov 17, 2015

kxcrl commented Nov 17, 2015

davidgoli commented Nov 20, 2015

kxcrl commented Nov 20, 2015

davidgoli commented Nov 20, 2015

hhff commented Nov 21, 2015

kxcrl commented Nov 21, 2015