Add middleware per route #128
Conversation
lib/mapper.js
Outdated
| @@ -91,7 +97,7 @@ module.exports = function mapper (routePlan) { | |||
| return routes | |||
| } | |||
|
|
|||
| function buildPath (route) { | |||
| function buildPath(route) { | |||
There was a problem hiding this comment.
Can you fix this (and the other linting errors?) Get the build passing....
|
Hey @tswaters, I wish to add my changes to the doc but I want to hear your opinions first about the changes I have made, remember you have not replied in the discussion we had in the issues section |
lib/adapters/log.js
Outdated
| @@ -2,7 +2,7 @@ | |||
|
|
|||
| var _ = require('lodash') | |||
|
|
|||
| module.exports = function log (options, context, auth, routes, done) { | |||
| module.exports = function log (options, context, auth, middleware, routes, done) { | |||
There was a problem hiding this comment.
Don't change the api contract between seneca-web and adapters. Remove middleware parameter here
lib/mapper.js
Outdated
| @@ -66,6 +66,12 @@ module.exports = function mapper (routePlan) { | |||
| route.autoreply = false | |||
| } | |||
|
|
|||
| // If there are middleware then set it | |||
There was a problem hiding this comment.
Above this section, if routeSet.middleware has been provided, attach it to defaultRoute similar to how it is set below.
web.js
Outdated
| @@ -9,6 +9,7 @@ var opts = { | |||
| context: null, | |||
| adapter: LogAdapter, | |||
| auth: null, | |||
| middleware: null, | |||
There was a problem hiding this comment.
move this option to the options key.
web.js
Outdated
| // they aren't stringify friendly | ||
| opts = extend(opts, _.omit(options, ['context', 'auth'])) | ||
| // Avoid deepextending context, middleware and auth, they aren't stringify friendly | ||
| opts = extend(opts, _.omit(options, ['context', 'auth', 'middleware'])) |
There was a problem hiding this comment.
remove middleware from here. extending functions should be fine. To be honest, I'm not sure why auth is here, I think that might be a HAPI thing that requires a complicated object.... Context definitely shouldn't be extended as this could be the entire web framework, and would be slow (also would likely blows up with circular references in koa1/koa2).
web.js
Outdated
| opts.context = options.context || null | ||
| opts.auth = options.auth || null | ||
| opts.middleware = options.middleware || null |
There was a problem hiding this comment.
Change this to something like....
if (options.middleware) {
opts.options.middleware = options.middleware
}this would allow for shorthand providing options at the root object instead of inside options.
web.js
Outdated
| adapter.call(seneca, options, context, auth, routes, done) | ||
| // Call the adaptor with the mapped routes, context to apply them to and | ||
| // instance of seneca and the provided consumer callback. | ||
| adapter.call(seneca, options, context, auth, middleware, routes, done) |
There was a problem hiding this comment.
Remove the middleware parameter.
web.js
Outdated
| locals = { | ||
| context: context, | ||
| adapter: adapter, | ||
| auth: auth, | ||
| middleware: middleware, |
web.js
Outdated
| function init (msg, done) { | ||
| var config = { | ||
| context: opts.context, | ||
| adapter: opts.adapter, | ||
| routes: opts.routes, | ||
| auth: opts.auth, | ||
| middleware: opts.middleware, |
|
Just noticed that Also, don't be afraid to add yourself to |
There was a problem hiding this comment.
Just went through a few of these to add support - pretty straight forward for the most part. Only weird one was connect becuase it doesn't have a simple api for an array of middleware so I had to get creative.
Got up to hapi and I realized how we assign route.middleware might need some work.
In hapi, there is no concept of middleware. There is a similar concept of pre requests. (you might need to search for "route.options.pre", as their anchor linking seems broken atm).
This pre array looks different than middleware in that you can nested middleware differently. I think we should be OK passing middleware directly as pre, except for one problem -- right now we are using _.flatten([middleware]) to coerce it to an array. This has the side effect of flattening the array - and there's an expectation that hapi should support nested arrays.
It may be that we replace this with --
route.middleware = _.isArray(value.middleware) ? value.middleware : [value.middleware]
1 similar comment
|
This looks pretty close.... I'm going to close this PR, but merge your branch into Once this is done, we need to get the web-adapters updated... as mentioned above I've already done a few of them and I see you started with the |
|
Thanks you @kasongoyo for all your hard work! I'm going to close this & move it to #130 Going to sit on this for a few days probably, come back and take a fresh look & hopefully get a new release out on the weekend sometime. |
|
Thanks @tswaters for your assistance. Thanks more for improving docs regarding middleware feature. The doc is real nice |
This pull request introduce a way to pass middleware instanc when defining route so that it can be utilized adapters when adding route to express or hapi context.