Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Allows filters to pass additional arguments to next() #114

Closed
wants to merge 4 commits into from

3 participants

@redking

Hey,

The before filter example in the Locomotive documentation shows data attached as a pseudo-private attribute on this that is later used in the rendering callback.

I was wondering if there was a reason why the before filter cannot simply pass data via the second argument on the next callback? It seems as if it would make the code a little neater, and prevent accidental overwriting of existing this attributes (a potential problem when multiple developers with different Node/express abilities are working on the same codebase).

I've included a pull request, in case this is simple to resolve.

@jaredhanson
Owner

I'm not seeing how the pull request relates to what you are suggesting. It doesn't seem to do anything with before filters.

In any case, could you expand on your suggestion a bit more. How would you handle multiple before filters that each pass arguments to next? They need to be merged together in some fashion, in which case conflicts could arise. Do you want them merged into res.locals eventually, so they are available in views? Ultimately that's what assigning to this does, so its just one more way to do effectively the same thing.

I'm intrigued by the idea, just curious to see if it actually addresses the problem.

@redking

For example, suppose I have a controller like this

controller.main = function(){
 console.log(arguments);
 console.log(this._data);

 if(this._err) {
  // Do something
 } else {
  this.data = this._data;
 }

 this.render();
}

controller.before('main', function(next){
 var _this = this;

 Service.fetch(function(err, data){
  _this._err = err;
  _this._data = data;
  next(err, data);  // arguments after 'err' are ignored
 });
});

Assuming err is undefined in the fetch, the controller.main callback will log an empty object (for arguments) and then the data returned from Service.fetch.

However with the pull request applied, the controller.main callback will log {0: null, 1: [data]} for arguments.

Thus I can rewrite the code like this

controller.main = function(err, data){
 if(err) {
  // Do something
 } else {
  this.data = data;
 }

 this.render();
}

controller.before('main', function(next){
 Service.fetch(next);
});

So in this example, assigning to this (or res.locals) is something performed in the main controller callback; the before filter is there to prepare some data or call a related service.

This is obviously a simplified example; I hadn't considered the case of multiple before filters.

The reason I raise it is that I'd noticed a proliferation of this and underscored variables in controller filters written by developers in my team, such that the code was becoming a little hard to read.

I'll think a little more about the case of multiple before filters ...

@jaredhanson
Owner

Gotcha. I'd be in favor of this.

It's not a near-term priority for me, but I'd gladly merge a pull request that considered multiple before filters and had unit test coverage. Feel free to take it on, and if you do, work of the master branch (which is now 0.4.x). It's been refactored significantly since 0.3.x, and I don't intend to backport any patches.

@redking

No problem; I'll try to find time to work on it over the coming week.

One solution for the multiple before filter scenario would be to treat the controller.main callback exactly like the final callback in async.parallel i.e. the data argument in the above example would be an ordered array of the responses from the before filters.

How would that sound?

@jaredhanson
Owner

I think maybe the async.waterfall approach is more sensible. If any error occurs in the before filter chain, the chain should stop and jump to error handlers (which is consistent with current behavior). Note that this means the err params don't have to be explicitly passed to the filters or action. Then, the result to controller.main is the result of the last before filter.

This works well with your service example, and, lets say you had another service that needed to fetch data first:

controller.before('main', function(next){
  ServiceOne.fetch(this.req.foo, next);
});

controller.before('main', function(data, next){
  ServiceTwo.fetch(data.bar, next);
});

controller.main = function(data){
  this.data = data;
  this.render();
}

controller.after('main', function(err, req, res, next){
  // respond to any errors
});

If the second filter wanted to carry through some of the first filters data, it'd have to munge that explicitly:

controller.before('main', function(data, next){
  ServiceTwo.fetch(data.bar, function(err, data2) {
    if (err) return next(err);
    data2.lorem = data.lorem;
    return next(null, data2);
  });
});

But I think that's probably fine in this case.

Thoughts? I definitely like this, and I can see it reducing a lot of my own app code!

@redking

Yeah, that makes more sense! I'll take a closer look at the 0.4.x branch tomorrow, and how this kind of change might work.

The example you give above is also probably the sort of thing you should use in the docs as, otherwise, with this change, the current example becomes

PhotosController.before('show', function(next) {
 Photo.findOne(this.param('id'), next);
});

and users might wonder why you wouldn't just put that code directly in show.

@jaredhanson
Owner

Agreed. FYI - the 0.4.x branch no longer exists, as it has been merged to master. Work continues from there.

@redking

Are you against the actual use of the async library in the implementation, or would you prefer not to add another dependency?

@jaredhanson
Owner
@redking

No problem; I should have time to do a pull request + tests for next week.

@coveralls

Coverage Status

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

@coveralls

Coverage Status

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

@redking

Apologies for the spam in this issue - my own fault, I forgot these commits were automatically rolled into the pull request. I'll make a pull request on a fresh fork once the implementation is complete

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Nov 28, 2013
  1. Allows filters to pass additional arguments to next()

    Cormac Flynn authored
Commits on Dec 2, 2013
  1. Upstream changes

    Cormac Flynn authored
  2. Adding .idea to .gitignore

    Cormac Flynn authored
This page is out of date. Refresh to see the latest.
Showing with 4 additions and 1 deletion.
  1. +3 −0  .gitignore
  2. +1 −1  lib/locomotive/controller.js
View
3  .gitignore
@@ -7,3 +7,6 @@ npm-debug.log
# Mac OS X
.DS_Store
+
+# Webstorm
+.idea
View
2  lib/locomotive/controller.js
@@ -548,7 +548,7 @@ Controller.prototype._invoke = function(action) {
if (!filter) {
// filters done, invoke action
try {
- self[action]();
+ self[action].apply(self, arguments);
return;
} catch (ex) {
return self.error(ex);
Something went wrong with that request. Please try again.