Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Update before filters to pass data as a second argument to next #116

Merged
merged 3 commits into from

3 participants

@redking

This pull request resolves #114.

You can now optionally pass a second data argument to next in before filters. For example

var err;

controller.before('show', function(next) {
  var data = {
    band: 'Counting Crows'
  };
  next(err, data);
});

controller.before('show', function(next, data) {
  data.album = 'August and Everything After';
  next(err, data);
});

controller.show = function(data) {
  this.band = data.band;
  this.album = data.album;
  this.render();
}

The data argument is completely optional; existing code can continue to attach attributes to this, and does not need to change its filter function signatures.

The potential signatures for a before filter are now

  • function(next)
  • function(next, data)
  • function(req, res, next)
  • function(req, res, next, data)

One thing to note - I originally modified the after filter code so it would have the same behaviour. However, this introduced an issue for after callbacks that catch all errors. With the additional data argument, the signatures function(req, res, next, data) and function(err, req, res, next) had the same "arity". Thus either we would have to make data a required fifth argument for all error callbacks, or change the way in which after error callbacks are defined. I wasn't happy with either option; in any case, this functionality is only really of use in the before filters, in my opinion.

I've put the 'after' code aside for the moment; I can make a subsequent pull request if you decide you'd rather the filters to behave identically, or if you can think of a better way of handling this.

@coveralls

Coverage Status

Coverage remained the same when pulling d0fc51d on redking:master into f834818 on jaredhanson:master.

@redking

@jaredhanson I have time this week btw so if there's anything preventing this from being in 0.4.x let me know and I can work on it. I'm keen to see it merged for this release, if that's at all possible. Thanks!

@jaredhanson
Owner

Could you merge in the upstream master and fix any conflicts? Shouldn't be anything major, but I did delint and rename the files to get rid of the locomotive directory under lib. I've almost wrapped up my to-do list and am hoping to cut 0.4.0 this week.

@redking

I just pulled in the upstream changes; there were no conflicts.

@coveralls

Coverage Status

Coverage remained the same when pulling 409664b on redking:master into b39bce8 on jaredhanson:master.

@jaredhanson
Owner

Awesome, thanks! I'll review it in the next day or two.

@jaredhanson
Owner

OK, I've got a data-filters branch, where I've merged in your commits.

(I did something stupid and delinted the test files, before merging, which caused all kinds of conflicts with the test cases you added. So, I manually copied the test cases you wrote into a separate file: test/controller.filters.before.data.test.js I kind of want to separate out the tests logically anyway, so that wasn't so band. Can you glance over that file and make sure I copied out all the tests you expect.)

More comments in a bit, once I get a chance to read it over more carefully.

@redking

Since the tests are separate now, we can probably improve the slightly-awkward way I named the test cases, by putting describe('Controller#before with data argument', ..) at the top, and then using the names (and order) of your original before tests from then on.

Other than that, the tests all seem to be there

@jaredhanson
Owner

I've made a data-filters-alt branch, differing only by this commit: 4f5b87c

The motivation for this proposal is to pass data as the first argument, keeping the next callback the final argument, which is more idiomatic Node.

This would look something like:

controller.before('show', function(next) {
  var data = {
    band: 'Counting Crows'
  };
  next(null, data);
});

controller.before('show', function(data, next) {
  data.album = 'August and Everything After';
  next(null, data);
});

controller.show = function(data) {
  this.band = data.band;
  this.album = data.album;
  this.render();
}

Again, the data argument is completely optional; existing code can continue to attach attributes to this, and does not need to change its filter function signatures.

I also dropped the data argument from any Connect-style middleware signatures. Since no Connect middleware would actually use the data param, I figured this is not needed. Let me know if you had a case for this.

The potential signatures for a before filter are now

  • function(next)
  • function(data, next)
  • function(req, res, next)

Agreed on the points about the after filters: I don't think this feature is useful for them. This applies only to before filters, until someone can pinpoint a use case for data arguments in after filters.

Let me know you're feedback. Assuming no objections, I'll merge this into master for release with 0.4.0.

@redking

I have no objections; I implemented the data functionality in connect-style filters purely for consistency, without considering if it would actually be useful :) So your proposal is much neater

@jaredhanson jaredhanson merged commit 409664b into from
@jaredhanson
Owner

Merged to master, will be included in Locomotive 0.4.0. Many thanks!

@redking

No problem! Thanks for all your help, and work on the framework. Looking forward to the release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Dec 2, 2013
Commits on Dec 9, 2013
Something went wrong with that request. Please try again.