remove app.router and refactor middleware processing #1909
Conversation
|
This is the actual cleaned up PR for the major routing overhaul. All of the tests and examples have been updated to reflect the new behavior. No new examples have been added yet nor have new expressjs.com docs been written for this. I do encourage those with existing express apps to clone this and see how your existing express app runs. Ideally it should just work as the majority of the changes were in private APIs. Using |
| return next_layer(err); | ||
| } | ||
|
|
||
| var arity = layer.handle.length; |
|
If there are objections please state them. This will hang around till about midweek and if no one objects, then this code will marry the master branch. If you have an objection but don't have time to say why yet, please just say that so you don't sit in silence :) |
| * @api public | ||
| */ | ||
|
|
||
| app.router = function(){ |
There was a problem hiding this comment.
is this even necessary? why would someone need this in 4.0?
maybe we can just make this a getter with a deprecation notice and return a noop as well as remove "docs" on it. it would make it more obvious that it's deprecated.
There was a problem hiding this comment.
This was suppose to be a way to get the app's router (as it says). The thinking is that you should be able to access that object in the event there are methods on it we don't proxy via app... maybe that is all useless thinking.
There was a problem hiding this comment.
yeah i think tj had an issue for that. i'm not sure if it's necessary though. i just think it's bad if we use the same name for a property but use it completely differently. better to just have a completely different name. even a public ._router is okay to me as it tells me "it's internal so i shouldn't have to touch it, but it's there".
There was a problem hiding this comment.
yea it was just a legacy thing for mounting router in the desired spot, shouldn't be an issue if we're doing per-router-middleware
|
can't see anything wrong right now. i'll look through again later. |
|
I have updated the PR with changes from comments
The decisions on |
|
Would love to get a LGTM on this or a no so I can move on to some of the other items :) This item is a blocker for fully removing the connect dependency. |
|
+1 |
|
👍 looks good to me :) |
This is an overhaul of middleware processing, Router and Route. Connect is no
longer used to process the middleware stack. This functionality has been
split into two parts: middleware stack and default error response.
The entry point for request processing is the `app.handle` method. It
sets up the default error response handle (to run in the event of no
other error handler) and then triggers the app router (instance of
Router) to handle the request.
The app router `handle` function contains the middleware dispatch layer
previously in the connect codebase. This layer handle the logic for
dispatching `.use` calls (stripping paths if needed). The app contains a
base router `app._router`. New routes can be created and `.use`d on this
router to organize routes into files.
Routers now have the following methods `.use`, `.all`, `.param` which
are all public.
Additionally, Routers have a `.route(path)` method which returns a new
instance of Route for the requested path. Route(s) are isolated
middleware stacks and contain methods for the HTTP verbs as well as an
`.all` method to act similar to middleware. These methods are chainable
to easily describe requirements for a route.
var route = Router.route('/foo'); // or 'app.route('/foo')'
route
.all(auth)
.get(function(...) {})
.all(more_checks)
.post(function(...) {})
Any Route and Router methods which accept handlers also accept error
(arity 4) handlers which will also behave as expected.
Finally, the `app.router` getter has been removed. Middleware and
handlers are run IN THE ORDER they are seen in the file. This means that
code which injected the `app.router` and then added error handlers (or
other middleware) will need to be updated to move those handlers after
any requests added on the app object. The examples have been updated
accordingly. This is the largest breaking change to codebases in this
commit.
This is an overhaul of middleware processing, Router and Route. Connect is no
longer used to process the middleware stack. This functionality has been
split into two parts: middleware stack and default error response.
The entry point for request processing is the
app.handlemethod. Itsets up the default error response handle (to run in the event of no
other error handler) and then triggers the app router (instance of
Router) to handle the request.
The app router
handlefunction contains the middleware dispatch layerpreviously in the connect codebase. This layer handle the logic for
dispatching
.usecalls (stripping paths if needed). The app contains abase router
app._router. New routes can be created and.used on thisrouter to organize routes into files.
Routers now have the following methods
.use,.all,.paramwhichare all public.
Additionally, Routers have a
.route(path)method which returns a newinstance of Route for the requested path. Route(s) are isolated
middleware stacks and contain methods for the HTTP verbs as well as an
.allmethod to act similar to middleware. These methods are chainableto easily describe requirements for a route.
var route = Router.route('/foo'); // or 'app.route('/foo')'
route
.all(auth)
.get(function(...) {})
.all(more_checks)
.post(function(...) {})
Any Route and Router methods which accept handlers also accept error
(arity 4) handlers which will also behave as expected.
Finally, the
app.routergetter has been removed. Middleware andhandlers are run IN THE ORDER they are seen in the file. This means that
code which injected the
app.routerand then added error handlers (orother middleware) will need to be updated to move those handlers after
any requests added on the app object. The examples have been updated
accordingly. This is the largest breaking change to codebases in this
commit.