change http server callback function identity to match the normal api

[max-mapper]

I wanted to do this (so that I could use the many functions on NPM that accept the (req, res) identity):

router.get('/', function(req, res) {
  res.end('hello')
})

instead of this

router.get('/', function() {
  this.res.end('hello')
})

there might be a better way to implement this but given the level of abstraction inside the director codebase I couldn't figure out anything more elegant. I also made sure all the tests still pass

[jfhbrook]

I think the args coming in are supposed to be the chunks that got matched by any patterns in the route.

I think.

[max-mapper]

the matches still get passed into the callback with this patch

[rvagg]

That'll break the api for all of us using director and expecting the arguments to be in a particular place. Unless you change it to args.concat([thisArg.req, thisArg.res]) so req, res are always last. Or you introduce a new option to change the behaviour.

[max-mapper]

a new option would be the way to go, i'll look into it when i get some more time or would welcome someone more familiar with the director repo to give it a shot

[max-mapper]

Also it would be nice if there was a test in director that enforced this API

Sent from my iPhone

On Dec 20, 2012, at 12:37 AM, Rod Vagg notifications@github.com wrote:

That'll break the api for all of using director and expecting the arguments to be in a particular place. Unless you change it to args.concat([thisArg.req, thisArg.res]) so req, res are always last. Or you introduce a new option to change the behaviour.

—
Reply to this email directly or view it on GitHub.

[pksunkara]

I agree with having an option

[max-mapper]

[indexzero]

@maxogden I've been wanting to make this change for a while. Eventually director@2.0.0 will have methods which confirm to the signature expected by request events in http[s].Server instances.

This is, however, massively breaking to all existing director code and needs to be rolled out slowly to users on the way to 2.0.0. It also needs to be consistent across all three usage scenarios:

• browser
• cli
• http

While the meaning of req and res inhttpforfunction (req, res) { ...` is well understood it is not as much so for the former two. Do you have thoughts around what they could be?

[max-mapper]

@indexzero well you would never have request and response objects in the browser or cli, only http, so it doesnt make sense for the 2 non http use cases.

the requirement of needing matches to be the first argument across all use cases moves the director into jack of all trades territory because the tradeoff breaks interoperability with the simple (req, res) pattern so that director can instead deliver better cross-usage scenario interoperability.

I personally dont use director in cli or browser and would rather have the http behavior act like node core, so maybe an initialization option to change the signature is the best way forward here.

something like:

new director.http.Router({httpCallbacks: true})

I'll try to implement this and add it to this pull req

[max-mapper]

:( Router already takes an object but it isnt a generic options object https://github.com/flatiron/director/blob/master/lib/director/http/index.js#L21

[indexzero]

@maxogden One of the core goals of director is to create a common abstraction across these three routing scenarios since they are all actively used @nodejitsu and to reuse routes across them. If we have a core and expected inconsistency between them we will be farther from our goal.

Although there is no req and res in browser and CLI scenarios perse se, I think it's better to conceptualize them as input and output.

When this came up previously in conversation the thoughts were something like:

browser

  function (input, output) {
    console.log(req) // { url: document.window.location.host, path: ['foo', 'bar'] } 
  }

CLI

  function (cmd, log) {
    console.log(cmd) // { path: ['foo', 'bar'] }
  }

Going back to my roots here I used to maintain journey with @cloudhead. The thing that made journey preferable to me over express in the early days was the mapping between regular expressions and function scoping.

e.g.

  router.path(/apps\/([\w\-]+)/, function () {

    router.path(/([\w\-]+)\/snapshots/, function () {
      //
      // List snapshots
      //
      this.get(function (username, appname) {
        //..
      });

      //
      // List snapshots
      //
      this.put(/([\w\-]+)/, { stream: true }, function (username, appname, id) {
        //
        // Pipe the snapshot (this.req) somewhere
        //
      });
    });
  });

Since there is no explicit naming provided to these regular expressions in the routing table they end up having to be stored as an Array (as is done in express in req.params) which are extremely unhelpful and very brittle when refactoring or reusing routes.

e.g. In the above example

  //
  // I prefer descriptive variable names
  //
  assert.equal(username, req.params[0]);
  assert.equal(appname, req.params[1]);
  assert.equal(snapshotId, req.params[2]);

Given the importance of matching core node http method signature the compromise here I see here is to stop allowing regular expressions as parameters to Router.prototype.on. In addition to avoiding this confusion around Arrays, internally this would simplify a lot of boilerplate where we handle both cases.

If users wish to use a regular expression they must do so via router.param. For example, the above example would be equivalent to:

e.g.

  router.param('username', /([\w\-]+)/);
  router.param('appname', /([\w\-]+)/);
  router.param('snapshotId', /([\w\-]+)/);

  router.path('/apps/:username', function () {
    router.path(':appname/snapshots', function () {
      //
      // List snapshots
      //
      this.get(function (req, res) {  // or maybe req, res, params
        // req.username
        // req.appname
      });

      //
      // List snapshots
      //
      this.put('/:snapshotId', { stream: true }, function (req, res) {
        //
        // Pipe the snapshot (this.req) somewhere
        //
        // req.username
        // req.appname
        // req.id
      });
    });
  });

[indexzero]

@maxogden @pksunkara I think in my discussion of the larger issue I neglected to make clear that this is fine if we make it a configurable option turned off by default.

It's a step on the road to director@2.0.0.

[indexzero]

Oh yeah and thanks @maxogden 👍