Reduce duplication in route #88

davidgoli · 2015-09-14T00:05:01Z

This branch is motivated in anticipation of some upcoming bugfixes. The state of the route is difficult to reason about due to numerous duplications and minor inconsistencies between the initial load and page 2+ loads. This branch attempts to bring them into line with each other. No tests have been changed, because no behavior has been changed.

hhff · 2015-09-15T19:37:25Z

addon/mixins/route.js

+    }
+
+    this.set(this.get('_modelPath') + '.reachedInfinity', true);
+    if(this.infinityModelLoaded) {


this.infinityModelLoaded is always going to be undefined here - it's not used anywhere else. We should use _canLoadMore

we're checking _canLoadMore as the first line of this method, this merely copies the previous lines 242:247 and 257:259

hhff · 2015-09-15T19:45:49Z

Thankyou @davidgoli ! I've been putting this work off for a while now. Thankyou a million!

davidgoli · 2015-09-16T14:33:48Z

I'm traveling for the next week or so, so my response time may be a little slow, but I'll get to these issues ASAP.

davidgoli · 2015-09-16T20:51:48Z

addon/mixins/route.js

+
+    promise.then(
+      newObjects => {
+        this.updateInfinityModel(newObjects);


in my other PR, all of this could go into _notifyInfinityModelUpdated

davidgoli · 2015-09-17T17:52:31Z

Rewrote the route test to be more explicit about what failed when a failure happens. Also added an assertion to ensure the page number is correct (converted from assert.ok(params.pageNumber) to assert.equal(params.pageNumber, expectedPageNumber)

davidgoli · 2015-09-17T17:54:34Z

It would be nice to standardize, when an asserted value is a boolean, to always use assert.ok or assert.equals(value, true). Right now it's arbitrarily inconsistent.

davidgoli · 2015-09-17T18:36:50Z

addon/mixins/route.js

+
+    this._loadNextPage();
+
+    return false;


So I don't think it's actually necessary to return false; from this method. The action itself does not return false, or return the value of this method, so it is probably unnecessary.

personally, I think it should always return a promise

davidgoli · 2015-09-17T20:34:15Z

@hhff OK, I think I'm done with the code churn here, pretty satisfied with how it's factored now. Please take a look at let me know if there's anything here you'd like me to change or clarify.

davidgoli · 2015-09-17T20:35:00Z

In addition, this PR incorporates the code change in #89 and supercedes that branch.

hhff · 2015-09-19T19:54:00Z

addon/mixins/route.js

@@ -102,6 +102,12 @@ export default Ember.Mixin.create({
   */
  totalPagesParam: 'meta.total_pages',

+  actions: {
+    infinityLoad() {
+      this._infinityLoad();


lets return here

davidgoli · 2015-09-20T02:33:47Z

I thought infinityModelUpdated and infinityModelLoaded were supported hooks? They're in the documentation and probably also depended on in userland...

hhff · 2015-09-21T15:28:58Z

oh yes they are sorry I forgot that Ember 2.0 requires hooks to be defined for them to run... Ember 1.x did not - that's actually all good!

I need to find time to do one last look over all of this (this week sometime - this is a huge refactor so I wanna be certain we're good) but thankyou so so much for this amazing work @davidgoli

davidgoli · 2015-09-29T23:07:24Z

@hhff merged in master

hhff · 2015-10-02T11:21:18Z

addon/mixins/route.js

@@ -283,17 +278,61 @@ export default Ember.Mixin.create({

   @method updateInfinityModel
   @param {Ember.Enumerable} newObjects The new objects to add to the model
-   @return {Ember.Array} returns the updated infinity model
+   @return {DS.RecordArray} returns the updated infinity model
   */
  updateInfinityModel(newObjects) {


updateInfinityModel is a public hook that current addon consumers are using to modify what happens when a payload is received.

By moving all of this functionality here, we're introducing breaking changes to users who are using this hook, and we're making the hook less useable.

Ember Infinity had over 250,000 downloads on NPM last month - this refactor is fantastic, but for such a simple lib we really can't be introducing breaking changes!

Fair enough. I made this change because the goal is to call updateInfinityModel not just after page 2+ but also on page 1 (see #90). I do think at a minimum we'll want to ensure infinityModelUpdated gets called.

On that topic, updateInfinityModel is a weird concept to me. What is it? Is it a hook? Is it a call to super? Particularly since this is a mixin, ideally the API would only use hooks that don't depend on a call to super, so users don't have to worry about return value or messing up functionality. The problem with the infinityModelUpdated hook however is that it's called outside of the current run loop, so that bound params may not be updated from new results before the next page load is triggered. We could leave updateInfinityModel and infinityModelUpdated as is, but we'd still need a hook that lets you update the bound params within the same run loop.

I agree that we shouldn't introduce breaking changes in this PR, but I would almost advocate for deprecating the current usage of updateInfinityModelsince it's inconsistent, and moving towards a new hook.

I'd also advocate for moving currentPage into the new updateBoundParams (or whatever) hook, and treating it as just another bound param. In our cursor-based infinity list, for example, the server doesn't even use the page number, it's just along for the ride.

Not in this PR though of course

Not sure why updateInfinityModel ever returned anything, the return value isn't used in calling code... but I'll maintain that functionality

we should also add a test to ensure that we preserve the functionality you're concerned about... it's not currently tested

hhff · 2015-10-02T11:27:21Z

Thanks @davidgoli - added a comment about a breaking change above.

I also love all the work you're doing refactoring the test suite! That's going to close an issue we've got open for upping the code-climate.

However because it's such a huge refactor of the main codebase, I'd prefer we kept the old test suite for this PR, and move the new test suite refactor to a subsequent PR. The old test suite hasn't let an regressions happen thus far, and I'd be more comfortable merging this if it's passing.

davidgoli · 2015-10-02T22:06:22Z

@hhff re: the test suite refactor, sure thing, I did all of that work in its own commit so it should be easy to split that off into its own branch.

davidgoli · 2015-10-03T00:14:55Z

@hhff Not sure what's going on with the build, looks like an issue with the ember-data remote? It's not a test failure. I suspect this is happening across all builds right now.

bower EINVRES       Request to https://bower.herokuapp.com/packages/ember-data failed with 503
The command "bower install" failed and exited with 1 during .

davidgoli · 2015-10-05T02:32:21Z

@hhff comments addressed

davidgoli · 2015-10-05T02:33:24Z

addon/mixins/route.js


-    return infinityModel.pushObjects(newObjects.get('content'));
+    return newObjects;


on this first page, this array becomes the infinityModel that is returned on subsequent calls

hhff · 2015-10-08T11:28:50Z

Hi @davidgoli !

@mkorfmann & I did a review of this yesterday - we think it's so so good and we're so thankful for your work here!

This is our only suggestion (for readability):

This also means the updateInfinityModel hooks stays exactly the same.

  updateInfinityModel(newObjects) {
    let infinityModel = this._infinityModel();
    return infinityModel.pushObjects(newObjects.get('content'));
  },

  _nextPageLoaded(newObjects) {
    const totalPages = newObjects.get(this.get('totalPagesParam'));
    this.set('_totalPages', totalPages);

    let infinityModel;
    if (this.get('_firstPageLoaded')) {
      infinityModel = newObjects;
      this.set('_firstPageLoaded', true);
    } else {
      infinityModel = this.updateInfinityModel(newObjects);
    }

    this._notifyInfinityModelUpdated(newObjects);

    const canLoadMore = this.get('_canLoadMore');
    infinityModel.set('reachedInfinity', !canLoadMore);

    if (!canLoadMore) {
      this._notifyInfinityModelLoaded();
    }

    return infinityModel;
  }

davidgoli · 2015-10-08T17:49:54Z

@hhff The problem with that change is that it regresses the fix for #90 (this is also the same as the fix in #89). If we're not going to call updateInfinityModel on the first page, then we'll need some other hook to fix that bug.

davidgoli · 2015-10-12T19:19:50Z

@hhff ping - thanks for taking the time to review this - just want to verify that making the change you are asking is a conscious decision not to fix #90. We can a) adopt the changes as I have them, b) add an additional hook, or c) make a conscious decision to leave the bug unfixed.

hhff · 2015-10-13T10:13:00Z

Thanks @davidgoli ! I think an Ember.run trigger like this._notifyInfinityModelInitialized will solve all our problems 👍

hhff · 2015-10-13T13:14:54Z

@davidgoli - also maybe checkout #77 - would that solve your problem in #90 ?

We plan to add that shortly after your PR gets in 👍

davidgoli · 2015-10-13T19:33:00Z

@hhff I've made the revert as you requested, let's merge this PR then we can hash out fixes to #90 separately. I also made a documentation change so updateInfinityModel's quirky behavior is documented. I think a PR like #77 (which is basically the new hook I was talking about) could also be the fix.

davidgoli · 2015-10-13T20:01:15Z

Also FYI I have that test refactor branch ready to go as soon as this gets merged!

ghost · 2015-10-13T20:07:58Z

Good stuff! Just want to give a big 👍 for the work you're doing here @davidgoli :).

BTW: Will #77 definitely help you resolving #90 and #89 or are you not sure yet?

Peace out :)

davidgoli · 2015-10-16T19:41:42Z

@mkorfmann I'm into discussing #77, I think it has some flaws (not least of which includes submitted with failing tests) but I think the general approach is ok. We can discuss on that PR though, is this one good to go?

ghost · 2015-10-17T11:06:20Z

@davidgoli All right! Thanks for submitting sum feedback on #77 :).

This PR must be good to go, I'm tempted too just merge it but I wan to get @hhff's absolution before.

hhff · 2015-10-20T20:43:44Z

Sorry all - been travelling!

ok @davidgoli - please squash the commits and I'll merge & release! this is 0.1.0 :)

davidgoli · 2015-10-20T21:03:56Z

@hhff squashed, but there's gonna be an extra commit, a merge from master, to resolve conflicts - FYI

kellyselden · 2015-10-20T21:05:50Z

@davidgoli You can rebase upstream master to get it back to one commit 😄

extract call inf model hook extract infinityModelLoaded into a shared method reduce number of args to methods expand documentation remove variable use a finally minor style tweaks extract assertions from initializer; fix logic around loadingMore only use _finishLoad to set reachedInfinity use startingPage for initial request have first page & page 2+ share all code consolidate response handling in updateInfinityModel extract getter for infinityModel; label private methods consolidate buildParams code wrap all assertions in a single method consolidate & simplify don't need extra call to _finishLoad reorder & simplify const-ify all variables change name of _finishLoad for consistency don't load more results if _canLoadMore is false ensure that the page number is reset when the model hook is run again make updateInfinityModel not private document param Allow setting starting page as 0. fix it for canary keep compatibility with pre-canary restore updateInfinityModel revert updateInfinityModel as requested

hhff · 2015-10-20T21:08:30Z

what @kellyselden said! <3

davidgoli · 2015-10-20T21:09:08Z

@hhff got it... it's done, should be gtg

hhff · 2015-10-20T21:10:11Z

will merge when it passes, you're a total monster @davidgoli

davidgoli · 2015-10-20T21:11:19Z

thanks for the review @hhff ! got another one coming right up

Reduce duplication in route

ghost · 2015-10-20T21:17:30Z

🎉

2015-10-20 23:13 GMT+02:00 Hugh Francis [email protected]:

Merged #88 #88.

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

hhff reviewed Sep 15, 2015
View reviewed changes

davidgoli reviewed Sep 16, 2015
View reviewed changes

davidgoli reviewed Sep 17, 2015
View reviewed changes

hhff reviewed Sep 19, 2015
View reviewed changes

hhff reviewed Oct 2, 2015
View reviewed changes

davidgoli reviewed Oct 5, 2015
View reviewed changes

hhff mentioned this pull request Oct 13, 2015

"Could not fetch Infinity Model" masks other errors #96

Closed

hhff added a commit that referenced this pull request Oct 20, 2015

Merge pull request #88 from sirenfoxlike/reduce-duplication

f38189f

Reduce duplication in route

hhff merged commit f38189f into adopted-ember-addons:master Oct 20, 2015

davidgoli deleted the reduce-duplication branch October 20, 2015 21:19


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

Reduce duplication in route #88

Reduce duplication in route #88

Conversation

davidgoli commented Sep 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hhff commented Sep 15, 2015

davidgoli commented Sep 16, 2015

Choose a reason for hiding this comment

davidgoli commented Sep 17, 2015

davidgoli commented Sep 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidgoli commented Sep 17, 2015

davidgoli commented Sep 17, 2015

Choose a reason for hiding this comment

davidgoli commented Sep 20, 2015

hhff commented Sep 21, 2015

davidgoli commented Sep 29, 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 Oct 2, 2015

davidgoli commented Oct 2, 2015

davidgoli commented Oct 3, 2015

davidgoli commented Oct 5, 2015

Choose a reason for hiding this comment

hhff commented Oct 8, 2015

davidgoli commented Oct 8, 2015

davidgoli commented Oct 12, 2015

hhff commented Oct 13, 2015

hhff commented Oct 13, 2015

davidgoli commented Oct 13, 2015

davidgoli commented Oct 13, 2015

ghost commented Oct 13, 2015

davidgoli commented Oct 16, 2015

ghost commented Oct 17, 2015

hhff commented Oct 20, 2015

davidgoli commented Oct 20, 2015

kellyselden commented Oct 20, 2015

hhff commented Oct 20, 2015

davidgoli commented Oct 20, 2015

hhff commented Oct 20, 2015

davidgoli commented Oct 20, 2015

ghost commented Oct 20, 2015