Support custom router

[bajtos]

Add a new parameter to express function to provide a custom Router
constructor.

Example:

var PromisedRouter = require('router-as-promised');
var app = express(PromisedRouter);

Rationale

This patch make it easier to experiment with different router implementations, as it enables developers (express users) to swap out the default Router implementation and replace it with their own.

• As promises are coming to ES6, there is an initiative to get promise support into express (see promises #2259). The last recommendation was to create a new router implementation supporting promises (promise-kernel).
• In LoopBack, StrongLoop's API framework based on express, we are experimenting with a different way of registering middleware (RFC: phased router [do not merge] strongloop/loopback#757). Our current solution involves overriding the default router used by express.

Discussion points

An existing workaround is to copy and edit app.lazyrouter to call a different Router constructor. This is very brittle as the copied version must be manually synchronized with any changes made in express.

Adding the first parameter to express() may be seen as controversial. Frankly, I don't really care about the API. I am happy to rework the PR to a different API as long as it allows me to change the Router ctor used by app.lazyrouter.

Example:

var app = expres();
app.Router = MyCustomRouterCtor;

// perhaps a different name would be better?
app.routerFactory = MyCustomRouterCtor;

@dougwilson @jonathanong Thoughts?

[dougwilson]

The answer to this has always been "no" because it adds more API surface for absolutely no reason: just add all your stuff on your custom router and then app.use(router) only. Allowing people to define their own custom router also means you have to re-implement the entire Express router API such that the app.VERB, app.param, etc. proxy methods still function the same, which is not a simple task.

[dougwilson]

As an extension to this problem, think of this: Express releases 4.15 and adds a new method to the router for a cool feature. A proxy is added to app, say app.coolthing() which calls app._router.coolthing(). Now people cannot use the coolthing if they are replacing the router until that third-party router supports coolthing, if it even wants to, and in the mean time, calling app.coolthing() will just throw because of it.

Replacing the router is not a thing that makes sense, especially when you just app.use(router) and simply attach all your routes to router instead of app and now both can live in co-existence.

[bajtos]

As an extension to this problem, think of this: Express releases 4.15 and adds a new method to the router for a cool feature. A proxy is added to app, say app.coolthing() which calls app._router.coolthing(). Now people cannot use the coolthing if they are replacing the router until that third-party router supports coolthing, if it even wants to, and in the mean time, calling app.coolthing() will just throw because of it.

I see your point. Basically if my patch was landed, it would mean that every time we added a new method to router, we would have to increase the major version, as we are breaking compatibility with custom routers. That would be really bad.

However, in my case, the custom router is inheriting from express.Router to prevent a problem like that, so I should not be affected by that.

Replacing the router is not a thing that makes sense, especially when you just app.use(router) and simply attach all your routes to router instead of app and now both can live in co-existence.

I disagree.

Firstly, I don't want to tell all LoopBack users that they can no longer use app.use and must call something like app.getBetterRouter().use instead.

Secondly, and this is more important, my custom router implementation runs routes registered via app.use() in the middle of the overall middleware chain. Let me illustrate this on an example:

app.use(handler1);
app.phase('init').use(handler2); // custom LoopBack API
app.use(handler3);
app.phase('final').use(handler4); // custom LoopBack API

// order of execution: 
//   handler2, handler1, handler3, handler4

I cannot add my custom router in the main middleware chain, as there is no (reasonably easy) way how to figure out what is the correct position for the custom router.

---

I understand your concerns about making router customization a public feature. Hopefully my explanation clarified my needs and you understand them too.

Can we come up with a way that will satisfy both of us? You don't want to make this a public API, I don't want to maintain a copy of lazyrouter implementation in LoopBack, as that opens even more holes.

How about making it a private undocumented thing intended for expert users only? Basically I need express application to read the Router constructor from a property I can change, instead of calling directly require.

[bajtos]

@dougwilson I have updated the PR to match my proposal above.

[dougwilson]

Firstly, I don't want to tell all LoopBack users that they can no longer use app.use and must call something like app.getBetterRouter().use instead.

I would say you are approaching the problem from the wrong direction: you shouldn't be returning an express instance as your app, but a derived instance that does what you need. This is the essence of why Express is going to be obsoleted soon.

Can we come up with a way that will satisfy both of us?

I 100% want to. I pined you on IRC to talk, but didn't get an answer back yet. If we need to setup a time to discuss, let me know, but the current proposal is too broken for Express as it is right now, but I can definitely work with you to get a solution. Express was designed in a specific way is all.

[bajtos]

I would say you are approaching the problem from the wrong direction: you shouldn't be returning an express instance as your app, but a derived instance that does what you need.

Let me extend that a little bit: I want to return an derived instance of express that is using a derived instance of express.Router as the default router.

Can we come up with a way that will satisfy both of us?
I 100% want to.

Thanks, I appreciate that. Not all OSS projects/maintainers share the same approach.

I pined you on IRC to talk, but didn't get an answer back yet.

I'll try to catch you up tomorrow. Or we can discus asynchronously here.

In the mean time, I'll revisit my current solution to see if there is a way how to simplify it and use a different mechanism for integrating it with express.

[Fishrock123]

Can we come up with a way that will satisfy both of us?

Hopefully! Framework collaboration has been the basis of our jshttp and pillarjs initiatives. :)

Firstly, I don't want to tell all LoopBack users that they can no longer use app.use and must call something like app.getBetterRouter().use instead.

Let me extend that a little bit: I want to return an derived instance of express that is using a derived instance of express.Router as the default router.

I would say you are approaching the problem from the wrong direction: you shouldn't be returning an express instance as your app, but a derived instance that does what you need. This is the essence of why Express is going to be obsoleted soon.

Yes, it would be better to think of exporting loopback, rather than express with loopback additions.

This doesn't mean it will become incompatible with express-based modules. Your framework will still have all of the express-exposed API available in it, if you like.

This honestly sounds like a request for pillarjs. Pillar is express "components" extracted for use by frameworks, designed to be interoperable and swappable with community components.

I think all of the express maintainers firmly believe the future is in a componentized "framework" that can be used to build these express-module-compatible frameworks.

You may be interested in a talk I did on this recently; we'd love to build this with anyone and everyone.

We'd be happy to discuss this more on something more casual if you'd like, such as IRC / Gitter, or even Skype. Github is also fine; but it can be harder to discuss sometimes. :)

[bajtos]

@Fishrock123 Your proposal sounds good and it will be a great solution in the long term. My problem is that I need a short-term solution that works with express 4.x as it is today. A small patch, not a major change...

[Fishrock123]

@bajtos It would be preferable to export loopback as a router, that will put you the most forward in terms of being able to easily adopt pillarjs in the future. :)

I should note that by "in the future" we mean at most 3 months. I'll let @dougwilson advise, but if you are currently monkey-patching things, the best option may be to continue to do so until pillar comes out in full force. The patched things are unlikely to change in severe ways during express 4, I think.

Any help on pillar would probably help accelerate it greatly, which sounds like it is desirable.

[jonathanong]

i'd rather extract all the prototype methods attached to node's request and response objects into its own repo and let people make their own framework with their own routers and such.

[dougwilson]

I'm going to accept a variant of this patch, even though the way you are injecting the router is a terrible idea and I guarantee is going to lead to people reporting bugs to loopback.

[dougwilson]

When strongloop/loopback#757 becomes more mature and is getting ready for release, I'll schedule a Express 4.x minor release to correspond with it. This will also give us time to make sure this is the right solution to your issue before merging it is now, and then perhaps you realizing there is a better way and we're left with something in Express we didn't particularly want. Does that make sense?

[dougwilson]

P.S. you can do this in Express 5.0 as an officially-supported thing:

var express = require('express')
var PromisedRouter = require('router-as-promised')

module.exports = function createApplication() {
  var app = express()
  var router = null

  // this is only necessary if you need to be lazy-initialized
  Object.defineProperty(app, 'router', {
    get: function getrouter() {
      if (router === null) {
        router = new PromisedRouter({
          caseSensitive: this.enabled('case sensitive routing'),
          strict: this.enabled('strict routing')
        })
      }
      return router
    }
  })

  return app
}

[bajtos]

When strongloop/loopback#757 becomes more mature and is getting ready for release, I'll schedule a Express 4.x minor release to correspond with it. This will also give us time to make sure this is the right solution to your issue before merging it is now, and then perhaps you realizing there is a better way and we're left with something in Express we didn't particularly want. Does that make sense?

Makes sense. I was thinking about your feedback in the evening and I came up with an idea for a different implementation, one that does not involve a custom router. I'll try it and see if it's better than the current proposal.

P.S. you can do this in Express 5.0 as an officially-supported thing:

That's much better.

FWIW, I still have a minor concern about that code, as you have to manually build a list of router options via multiple app.get calls. If a new option is added to express.Router, all apps building a custom router have to be updated with the new option.

IMHO, it would be nice to expose a function to build the default router settings object.

// in the app
  Object.defineProperty(app, 'router', {
    get: function getrouter() {
      if (router === null) {
        router = new PromisedRouter(this.buildRouterConfig())
      }
      return router
    }
  })

// in express
app.buildRouterConfig = function() {
  return {
    caseSensitive: this.enabled('case sensitive routing'),
    strict: this.enabled('strict routing')
  }
}

Just a food for thoughts, this discussion does not belong to this PR.

[dougwilson]

FWIW, I still have a minor concern about that code, as you have to manually build a list of router options via multiple app.get calls

Right, but you're assuming your custom router even has "strict" and "case-sensitive" settings at all, and it would preclude you from being able to let people control settings that are specific to your router using app.set, since you'll no longer have control over what settings to read out of Express.

this discussion does not belong to this PR

but why not? it all seems relevant to me :)

[bajtos]

Right, but you're assuming your custom router even has "strict" and "case-sensitive" settings at all, and it would preclude you from being able to let people control settings that are specific to your router using app.set, since you'll no longer have control over what settings to read out of Express.

In my case, the router is extending express.Router, thus is support all settings that express.Router does.

You can always control the router-specific settings, as you can add more stuff to the config object created by express, or don't use the default config object at all.

// extend
var config = this.buildRouterConfig()
config.myCustomOpt = app.get('my custom opt')
router = new PromisedRouter(config)

// replace
var config = {
  myCustomOpt: app.get('my custom opt')
};
router = new PromisedRouter(config);

[bajtos]

I had a nice discussion with @dougwilson about the different approaches for customising the router behaviour. He wrote down a simple proof-of-concept that works with the current express version and does not depend on too many internals - see his gist. Using some of his ideas, I was able to modify the LoopBack code so that it does not create any custom Router implementation - see app.lazyrouter in my branch.

As far as I am concerned, this issue can be closed.

Thank you all for your help.