Added the ability to pass in extra params that are (computed) properties

[ashrafhasson]

Hi @hhff, I've added the ability to pass in extra params that can be either static or dynamic. If the extra param is a property on the route, its value will be used for setting the extra param.

I hope this is an acceptable enhancement, I'm currently using this method to dynamically change the extra params I'm passing to the back-end.

[kellyselden]

What about people who already have an extra param and a route property with the same name, but don't expect this behavior?

[hhff]

Yeah was thinking about that last night. Only thing I could come up with was that binding could be declared as "infinityRoute.category". Don't love it tho

Any ideas @kellyselden / @ashrafhasson ?

[kellyselden]

Maybe a third option to infinityModel?

export default Ember.Route.extend(InfinityRoute, {
  aRouteProperty: 'theValue',

  model: function(params) {
    var boundParams = { aBoundProperty: 'aRouteProperty' };
    return this.infinityModel('user', { perPage: 5 }, boundParams);
  }
});

[ashrafhasson]

@kellyselden , @hhff yeah gotta admit I overlooked that but also thought it might be acceptable to introduce a breaking change, maybe not though.

[ashrafhasson]

@kellyselden I think this is actually a good option, it eliminates the guess work, is clean and doesn't break things. I'll make alterations and submit for review.

[hhff]

Works for me!

[hhff]

Can we a) use an ES6 fat arrow function in the forEach so we don't have to bind _this, and b) pull this out into a private function so we don't have to have it in both infinityModel & infinityLoad

[ashrafhasson]

@hhff sorry I seem to have broken the tests and in my haste forgot to run them. a) is done. b) how? I've tried to privatize and while this works it'll still fail the tests and travis. Appreciate a hint, thanks.

[ashrafhasson]

@hhff apologies, fixed it to call the private function.

[hhff]

Looks great to me! @kellyselden / @truenorth can you take a second look?

[kellyselden]

LGTM

[hhff]

@ashrafhasson - could we get some test coverage on this? Once we've got some tests I'm keen to merge!!

[kellyselden]

+1 test coverage

[ashrafhasson]

@hhff / @kellyselden will try to allocate time for adding couple tests over the weekend.

[mike-north]

I'll take a look at this today

[ashrafhasson]

Sorry for the delay in coming back to this, also testing is kinda new to me, here's what I've done pretty much following the existing tests as my example:

test('it uses bound params when loading more data', assert => {

  assert.expect(8);

  var RouteObject = Ember.Route.extend(RouteMixin, {
    model() {
      return this.infinityModel('item', {perPage: 1, startingPage: 2}, {category: 'feature'});
    },

    perPageParam: 'testPerPage',
    pageParam: 'testPage',
    totalPagesParam: 'meta.testTotalPages',
    feature: Ember.computed.alias('test'),
    test: 'new'
  });
  var route = RouteObject.create();

  var dummyStore = {
    find(name, params) {
      assert.equal('new', params.category);
      assert.ok(params.testPage);
      return new Ember.RSVP.Promise(resolve => {
        Ember.run(this, resolve, Ember.Object.create({
          items: [{id: 1, name: 'Test'}],
          pushObjects: Ember.K,
          meta: {
            testTotalPages: 3
          }
        }));
      });
    }
  };

  route.store = dummyStore;

  var model;
  Ember.run(() => {
    route.model().then(result => {
      model = result;
    });
  });
  // The controller needs to be set so _infinityLoad() can call
  // pushObjects()
  var dummyController = Ember.Object.create({
    model
  });
  route.set('controller', dummyController);

  assert.equal(true, route.get('_canLoadMore'));

  // Load more
  Ember.run(() => {
    route._infinityLoad();
  });

  assert.equal(false, route.get('_canLoadMore'));
  assert.equal(3, route.get('_currentPage'));
  assert.ok(model.get('reachedInfinity'), 'Should reach infinity');

});

what I'm not sure of is how to change the route's property and assert that another load uses the newer value of the computed property in this unit test but that might be an integration test scenario?

[ashrafhasson]

apologies all, I think I figured it out, does this test make sense:

...

  var RouteObject = Ember.Route.extend(RouteMixin, {
    model() {
      return this.infinityModel('item', {perPage: 1, startingPage: 1}, {category: 'feature'});
    },

    perPageParam: 'testPerPage',
    pageParam: 'testPage',
    totalPagesParam: 'meta.testTotalPages',
    feature: Ember.computed.alias('test'),
    test: 'new'
  });
  var route = RouteObject.create();

  var dummyStore = {
    find(name, params) {
      assert.equal(route.get('test'), params.category);
      assert.ok(params.testPage);
      return new Ember.RSVP.Promise(resolve => {
        Ember.run(this, resolve, Ember.Object.create({
          items: [{id: 1, name: 'Test'}, {id: 2, name: 'New Test'}],
          pushObjects: Ember.K,
          meta: {
            testTotalPages: 3
          }
        }));
      });
    }
  };

  route.store = dummyStore;

...

  // Load more
  Ember.run(() => {
    route._infinityLoad();
  });

  assert.equal(true, route.get('_canLoadMore'), 'can load even more data');
  route.set('test', 'hot');
  // Load even more
  Ember.run(() => {
    route._infinityLoad();
  });
  assert.equal(false, route.get('_canLoadMore'));
...

[kellyselden]

I don't think these 3 lines are necessary, they are testing the name resolution feature.

[ashrafhasson]

Hmmm, @kellyselden, assuming you meant 317-319, if I don't define a model, how would I test loading more data with the third option that 'infinityModel' would take to define the dynamic property?
The idea is that the two option hashes to 'infinityModel' are merged before submitting a request and this ensures the second option is passed on to the model. I also needed to set the startingPage to 1 and perPage to 1 so that I can load at least twice observing that the dynamic part got updatede on the second request.

[ashrafhasson]

@hhff, @kellyselden the test was updated and I removed the unnecessary tests, do you guys recommend I add something else to the test? Is there anything pending that I need to be aware of?

[hhff]

Looks good to me @ashrafhasson - can you merge master and squash commits?

[ashrafhasson]

@hhff rebased and squashed :)

[hhff]

THE MAN!!! thankyou so much for ur work - i know this PR has been hanging for ages @ashrafhasson

[hhff]

https://github.com/hhff/ember-infinity/releases/tag/v0.0.12

[kellyselden]

@ashrafhasson Nice work.

[ashrafhasson]

awesome, happy to have contributed and thank you for merging my PR.