RFC: phased router [do not merge]

[bajtos]

A prototype implementation of of a phased router - see #739.

Usage

// called inside "routes" phaseapp.use('/foo',function(req,res,next){/*...*/});// called amongst the first middlewareapp.phase('init').use(compress());// called after all routes were handledapp.phase('files').use(loopback.static('./public'));// install a "before" handlerapp.phase('init').before(serveFavicon(loopback.faviconFile));// create a route invoked before "regular" routes// note that `.after` and `.before` cannot be used for routes yetapp.phase('init').post('/foo',function(req,res){/* ... */});

Todo

• API for registering new phases and specifying their order
• Add more built-in phases (probably in loopback to keep the phased router less opinionated)
• Consider renaming error phase to end or final
• Move server-specific code from lib/application.js to a standalone file
• Contribute express enhancement allowing people to configure their own Router implementation to be used as the default router, without the need to copy & past app.lazyrouter function: Support custom router expressjs/express#2431
• Fix undefined phase in PhaseList.run - see PhaseList: fix run() and unit-tests loopback-phase#2
• Write a unit-test to verify that router properties like caseSensitive are correctly forwarded by DecoratedRouter
• Improve loopback-phase to support generic Phase implementations (any object with id and run can be used as a phase)
• Implement customization hooks in PhaseList.run
• Implement PhaseList.get(name) that throws a helpful error when the phase was not found

/to @raymondfeng@ritch please review, ideally by EOD. Tomorrow my time, I'll start moving the code to a new module strong-router and work on the missing items outlined above.

[raymondfeng]

Is this an optimized implementation of util.inherits()?

[bajtos]

Not really.

1. AFAIK util.inherits works for objects created via new. The router instances are functions, they use different mechanism.

2. With util.inherits, if you want to override a method and call the base class from your impl, you have to use apply or call, because you need to specify a different this object:

   Child.prototype.method=function(){Base.prototype.method.apply(this,arguments);}

   DecoratedRouter allows you to call methods without bind, because this._super and this are sharing the same set of properties.

   Decorated.prototype.method=function(){this._super.method();}

However, there is one thing that does not work:

Base.foo=function(){this.bar();}Decorated.foo=function(){// do somethingthis._super.foo();}vard=newDecorated();d.bar();// calls Base.foo, not Decorated.foo

I'll give this some more though tomorrow to see if I am able to come up with a better solution.

[ritch]

We need to move this outside the LoopBack repo... this is too much to land in the core. My initial impression is that this reinvents or adds to phases. I'd rather implement those features in the phase module.

Originally I didn't think we needed to build a custom router... but seeing this it makes sense. Although some things are not clear:

varphase=app.phase('init');

This should return a phase. AFAIK I didn't implement the following for phases:

phase.use(myMiddleware);

Since the myMiddleware handler will be passed a ctx object as the first param.

[ritch]

We can probably test the number of arguments to tell if the function is a middleware or a phase handler.

[bajtos]

We need to move this outside the LoopBack repo... this is too much to land in the core. My initial impression is that this reinvents or adds to phases. I'd rather implement those features in the phase module.

Agreed, I never intended to land this in loopback. It was just faster to implement the prototype in this repo, since I have everything in one place.

AFAIK I didn't implement the following for phases:

phase.use(myMiddleware);

No, that's my extension.

I'd like to make PhaseList more generic and support any object with id and run(ctx, cb) as a phase. See MiddlewarePhase - it implements all Phase methods. When the phase is executed, MiddlewarePhase delegates the execution to the express router.

We can probably test the number of arguments to tell if the function is a middleware or a phase handler.

Adding exceptions and special rules like that is IMO a road to hell. I'd rather make the phases more generic, so that it's easy to build any solution based on them.

[ritch]

Adding exceptions and special rules like that is IMO a road to hell. I'd rather make the phases more generic, so that it's easy to build any solution based on them.

Phases are already very generic. The following API makes it possible to build on top of phases without having to extend them for the middleware use case:

app.use('init',myMiddleware(options));

Psuedo implementation:

app.use=functino(phaseOrRoute,middleware){varroute=getRoute(phaseOrRoute);varphase=getPhase(phaseOrRoute);if(route){originalUse(route,middleware);}elseif(phase){phase.use(function(ctx,next){middleware(ctx.req,ctx.rex,next);});}// ...}

This leaves phases untouched and unaware of middleware. Creating a MiddlewarePhase means that other phase handlers will only have access to the req and res objects. The ctx object is useful for hanging objects that might be useful for other phase handlers.

However it doesn't support the same API as outlined at the top of the thread. It won't let you do before() or after()...

[ritch]

To rephrase my point above... I don't really mind what the implementation is as long as I am able to do the following:

// in a loopback componentapp.phase('auth').use(function(ctx,cb){getTheCurrentUser(ctx.req,function(err,user){if(err)returncb(err);ctx.set('user',user);cb();});});// in my appapp.phase('auth').after(function(ctx,cb){console.log(ctx.get('user').email);// => foo@bar.comcb();});

[raymondfeng]

Would use/before/after return the Phase object so that we can do the chaining of methods?

app.phase('my-phase').use(handler1).after(hander2);

I think as long as we agree on the apis, @bajtos can choose however he implements them.

[bajtos]

Here are the most important API decision we need to make:

1. Do we want to support all (future) features of Express router? Examples:

// scoped middlewareapp.phase('init').use('/subpath',handler);// routes - they always terminate the chain AFAIKapp.phase('init').post('/subpath',routeHandler);// request parametersapp.phase('init').param('user_id',function(req,res,next,id){User.find(id,function(err,user){if(err)returnnext(err);req.user=user;next();});});

2. Do we want to support all (future) features of Phases? Examples:

// manually run the phaseapp.phase('init').run(ctx,next);// a silly exampleapp.phase('init').toString();// what if this is added in the future?app.phase('init').clear();

Considering the main use case we have for middleware phases,
which is declarative registration of middleware provided by components,
I am inclined to answer "no" to both questions, at least for the initial
implementation.

3. Do we want keep the handler signature compatible with connect/express middleware?

// express compatibleapp.phase('init').use(function(req,res,next){ ... });app.phase('error').use(function(err,req,res,next){ ... });// phase/hook basedapp.phase('init').use(function(ctx,next){// ctx.req, ctx.res });app.phase('error').use(function(ctx,next){// ctx.err, ctx.req, ctx.res });

I am personally in favour of staying compatible with express middleware.

4. Regarding ctx.get and ctx.set used in @ritch's example above: the express convention is to store extra parameters on the req object. LoopBack adds the concept of a current context that can be used too. IMO we have already enough ways for passing stuff between callbacks, there is no need to introduce yet another one.

---

For the initial version, I am proposing to add a single new method to app API:

app.middleware(phaseName,handlerFn);

This name is preserving consistency with other loopback-provided
methods like app.model, app.dataSource and app.connector.

Usage:

app.middleware('init',compression());// phases.find('init').use(compression())app.middleware('before:init',serveFavicon());// phases.find('init').before(serveFavicon(...))app.middleware('after:files',loopback.urlNotFound());// phases.find('files').after(loopback.urlNotFound());

Encoding before/after into the phase name keeps the API concise and
also makes it easy to specify the before/after flag in the json-based
configuration.

A larger example:

// example by @ritch// in a loopback componentapp.middleware('auth',function(req,res,next){getTheCurrentUser(req,function(err,user){if(err)returnnext(err);req.user=user;loopback.getCurrentContext().set('user',user);next();});});// in my appapp.middleware('after:auth',function(req,res,next){console.log(req.user.email);// => foo@bar.comnext();});

[ritch]

I'm fine with app.middleware.

[bajtos]

Closing in favour of #767.