Get arguments

[lodewijkadlp]

I recently did a pull request that would've broken everything. Turns out I mixed old and new code. This is the code as I use it myself now. I tried catching GET parameters by ending with "**" but that didn't catch behind the "?", now choreographer parses the GET parameters (when present) and adds them as a last parameter.

Eg:

router.get('/register/*/*', function(req, res, username, password){ console.log(username + ':' + password); });

or

router.get('/register', function(req, res, args){ res.end(args.username + ':' + args.password); });

or

router.get('/register/*', function(req, res, lobby, args) { res.end('Entered lobby ' + lobby + 'arguments: ' + JSON.stringify(args));} );

As you can see it's fairly elegant. Someone who doesn't know it's there doesn't notice it. It allows slightly more "standard compliant" behavior of putting arguments behind the "?".

[laughinghan]

Node already has an excellent query string parser, but I see no reason why Choreographer would do that for you, since it's completely orthogonal to Choreographer's core routing functionality.

I would consider setting req.parsedUrl = parse(req.url) since Choreographer uses that and maybe you don't want to call it twice (I remember reading somewhere that Ryan Dahl wrote it in heavily optimized C, so maybe it's normally a slow operation?).

[gsf]

-1 on query parsing. Agreed that it's not the router's job.

+1 on exposing the parsed URL at req.parsedUrl since it has to be parsed already.

[lodewijkadlp]

Choreographer has a code-wise logical position to provide for this parsing. Choreographer calls a part of personal code with the arguments passed in-URI. It does this already! We've already have a hacked in argument passing via URL ('/*/') instead of respecting standard and using GET arguments. Since GET arguments are orderless and not intended for it they shouldn't be used for routing. I see no reason we should support a hacky way and not the standard way of passing arguments.

I'm using this for my site right now and it makes a lot of sense, it's easy, quick and simple. If you don't like it you'll never notice it's there. It's a 4 line addition that'll likely save you lines (it sure did to me). We could use the node-standard parser but even in the example non-standard processing is demo'd (supplying the same argument twice should overwrite, the latter argument being the actual value) and depending upon that code's consistency isn't wise when this suffices.

[laughinghan]

I won't comment on whether the arguments Choreographer passes in are "hacky", but there is precedent, Choreographer's routing API is based on Sinatra's.

Your GET arguments code does make a lot sense, and it does appear to be easy, quick, and simple. Its functionality is also completely orthogonal to Choreographer's. In my humble opinion, a piece of 4-line middleware in a stage after Choreographer would save you those same lines while loosening coupling and increasing cohesion over having both pieces of unrelated functionality being in your router.

[laughinghan]

Something that would help with such middleware and I think make sense as part of Choreographer would be a Sinatra-style before filter, e.g.:

router.beforeGet('/register', function(callback, args, req, res) {
  //parse the GET query
  var url = req.parsedUrl, query = {};
  url.query && url.query.split('&').forEach(function(pair) {
    var pairsplit = pair.split('=');
    query[pairsplit[0]] = pairsplit[1];
  });
  //and append that to the callback's arguments
  args.push(query);
  callback.apply(this, args);
});
router.get('/register/*', function(req, res, lobby, args) {
  res.end('Entered lobby ' + lobby + 'arguments: ' + JSON.stringify(args));
});

I haven't fully thought this through, what do you guys think?

[lodewijkadlp]

It depends on how I specify when to use the middleware. In the shape you presented I think it'd not be usefull for me. If I could just wildcard all GET requests it'd be fine.

I still disagree on it's function being orthogonal. Sure it's not routing but already Choreographer does more than routing. Really Choreographer brings a request to my code, so I can just write the handling code. The work it does for me is handeling (routing) requests. This is IMHO still very much calling my code when it should, in a convenient manner. It's specific to GET requests though, that might be grounds for exclusion I could agree with.

Before filters would likely not be worth the lines and complexity required to implement them properly. Maybe if you can do a filter for a type of request (beforeGet('*', function())) optionally limiting the URL to have to match. You can then provide my code as an example of a filter in the readme, it's a pretty useful snippet.

[gsf]

I don't like the look of beforeGet. Either include middleware in the server stack before passing control to your routes, affecting every request (but only acting on GETs, etc., as you wish), or include it in the route before the rest of the code there.

[gsf]

Also, regarding this query-parsing snippet, it lacks support for param arrays, alternative separator and assignment characters, and a maximum keys guard, all of which we can get in one line with the querystring module:

router.get('/register', function(req, res) {
  var query = querystring.parse(req.parsedUrl.query, ';', ':', {maxKeys: 100});
  // do things with your query object here
});

If we don't have req.parsedUrl, and you're fine with the defaults, the url library can do the job:

router.get('/register', function(req, res) {
  var query = url.parse(req.url, true).query;
  // do things with your query object here
});

Either of these approaches could be wrapped in a middleware function and applied as necessary. For example:

function parseQuery(req, res) {
  req.query = url.parse(req.url, true).query;
}

http.createServer(function(req, res) {
  parseQuery(req, res);
  // routes now have a query object at req.query
  router(req, res);
}).listen(5000);

I've been enjoying using https://github.com/creationix/stack, in which case things could look like this:

function parseQuery(req, res, next) {
  req.query = url.parse(req.url, true).query;
  next();
}

router.get('/register', stack(
  // apply parseQuery at the route level
  parseQuery,
  function(req, res, next) {
    // do things with req.query
  }
));

http.createServer(stack(
  // or apply parseQuery at the server level
  parseQuery,
  router
)).listen(5000);

[gsf]

And (sorry for the barrage) parseQuery could of course only act on GETs:

function parseQuery(req, res, next) {
  if (req.method == 'GET') {
    req.query = url.parse(req.url, true).query;
  }
  // work with or without a next callback
  if (next) next();
}

[laughinghan]

@lodewijkadlp I was definitely thinking the route in the before filter would be optional, defaulting to /, i.e. intercept all GET requests. As for orthogonality: when you say "Choreographer brings a request to my code, so I can just write the handling code", you're saying that routing was the only piece of middleware you needed (until you needed query parsing). But Choreographer still never did more than routing (until you added query parsing). It is indeed important to have middleware "calling [your] code when it should, in a convenient manner", but it's also important for this code to be organized to have minimal coupling and maximal cohesion, and if it does two different things it should do them in two different layers.

[laughinghan]

@gsf If you want to intercept all requests of a certain type, or to only do something for a particular route, then certainly a before filter would be unnecessary. However, I think a before filter makes sense if you want to organize middleware hierarchically, i.e if you want to insert middleware to only intercept certain subroutes: lets say you have a few related but insulated services on the same domain but with different top-level paths, /foo, /bar, and /baz, and service foo has methods /foo/beep and /foo/boop, and you want to insert middleware to deal with all foo methods but no /bar, /baz, or any other services' methods. Without a before filter you have two options:

• insert the middleware before calling choreographer, which necessitates routing logic to figure out whether the path starts with /foo and the middleware should intercept the request,
• or individually wrap your /foo/beep, /foo/boop, and all other /foo route handlers with the middleware,

both of which involve smelly duplication. In this situation, what I think you really want is for the router to handle routing requests through your middleware, hierarchically, before it reaches your handlers.

What I'm unsure about is whether this situation actually comes up; @lodewijkadlp seems to be saying they want this on all (GET) requests, so your solution would work fine.

Side note: I'm toying with before{Get,Post,etc.}([ path ], filter) vs before(verb, [ path ], filter). Maybe verb could be a space-delimited list, like jQuery event handlers, and even optional, the mandatory leading / on paths and whitelist of verbs would distinguish them. It doesn't seem far-fetched to me that you'd want to insert middleware to intercept /foo requests of all verbs, or all /foo GET and POST requests.

[gsf]

Support for hierarchical path namespacing reminds me of https://github.com/cloudhead/journey#paths -- are you thinking something along those lines?

You could also achieve something like this with express-style route control passing: http://expressjs.com/guide.html#passing-route%20control.

[lodewijkadlp]

Let's take this one step further!

All functions called are processing steps, right? Be they the one calling res.end or some in-between steps they're all just functions being processed.

Let's make the filter ('/', '/function//*', etc.) just that. And let's add a parameter, called the precedence integer. Let's add another, special, parameter that accesses a request-specific object (but remains between functions).

Now, in order of ascending precedence integers, process every function who's filter checks out as true.

If I say router.get('/', 1, function(req, res, obj) { obj.sample = 'samp'; } router.get('/', 10, function(req, res, obj) { res.end(obj.sample); } A GET request to / will always return samp.

This can be used to plug an authentication middleware to "/" or to "/auth/**" or the like without affecting other steps. It's also easily stackable (you want another preprocessing step? Sure no problem), it explicitly includes ordering of steps, and it's fairly easy. We can make the precedence variable default to 1, taking precedence as "first added, first processed".

I think this'll make the interface consistent and a lot more functional.

[gsf]

Precedence on "first added, first processed" I could see being useful. I'm not sure I understand the precedence integer, and I get the sense that there would be a number of edge cases around it.

[lodewijkadlp]

As with the z-index on elements a lower to 0 precedence integer causes priority. IOW: processing steps are ordered lowest precedence integer first. 0 before 1 before 2 before 3 before 4 before 5 etc.

Because things can have the same integer we should sub-order on first added first processed. This will define clear order of processing.

[laughinghan]

Can they be negative? Do they have to be integers?

I don't really see the advantage of specifying precedence over just filtering in the order the filters are defined. Rather than taking a step further, I'd like to step back as far as possible, and keep Choreographer minimalist. Can we simplify the before approach? Can we unify it with the existing routes, so maybe instead of matching routes in the order they're defined, route handlers are passed a continuation that will try another route?

[lodewijkadlp]

I think simpler and general (more powerful) is better. Side effects are not always fun. If I don't want order to matter then with precedence I can.

I'd argue it's already the simplification. It DOES unify the two. On Jul 25, 2012 8:36 AM, "Han" < [email protected]> wrote:

Can they be negative? Do they have to be integers?

I don't really see the advantage of specifying precedence over just filtering in the order the filters are defined. Rather than taking a step further, I'd like to step back as far as possible, and keep Choreographer minimalist. Can we simplify the before approach? Can we unify it with the existing routes, so maybe instead of matching routes in the order they're defined, route handlers are passed a continuation that will try another route?

---

Reply to this email directly or view it on GitHub: https://github.com/laughinghan/choreographer/pull/8#issuecomment-7241784

[laughinghan]

I think the unification is a great idea, and I'm trying to incorporate that. However, I think specifying the order your code runs in by a runtime integer rather than by ordering your code is a recipe for spaghetti code.

The problem with side effects is spooky action at a distance, stuff like "over in this code I have these 3 pieces of middleware running one after another, and it looks all fine and dandy, and over in this completely separate code I added a piece of middleware that looks innocuous enough, but gave it a precedence integer that got it inserted in between the middleware way over there and now sometimes this piece does something and those 3 over there stop working".

If you think specifying precedence with an integer is simpler and more powerful than just ordering your code, you could use a generalization that lets you order all of your code by precedence, like:

order(1, function() {
  router.get('/', function(req, res, obj) { obj.sample = 'samp'; });
});
order(10, function() {
  router.get('/', function(req, res, obj) { res.end(obj.sample); });
});

[lodewijkadlp]

Very well. It's just that typical code without side-effects doesn't mind about order (see Haskell). Precedence was an optional thing anyway. On Jul 26, 2012 6:09 PM, "Han" < [email protected]> wrote:

I think the unification is a great idea, and I'm trying to incorporate that. However, I think specifying the order your code runs in by a runtime integer rather than by ordering your code is a recipe for spaghetti code.

The problem with side effects is spooky action at a distance, stuff like "over in this code I have these 3 pieces of middleware running one after another, and it looks all fine and dandy, and over in this completely separate code I added a piece of middleware that looks innocuous enough, but gave it a precedence integer that got it inserted in between the middleware way over there and now sometimes this piece does something and those 3 over there stop working".

If you think specifying precedence with an integer is simpler and more powerful than just ordering your code, you could use a generalization that lets you order all of your code by precedence, like:

order(1, function() {
  router.get('/', function(req, res, obj) { obj.sample = 'samp'; });
});
order(10, function() {
  router.get('/', function(req, res, obj) { res.end(obj.sample); });
});

---

Reply to this email directly or view it on GitHub: https://github.com/laughinghan/choreographer/pull/8#issuecomment-7282138