Middleware hooks #24
Middleware hooks #24
Conversation
package.json
Outdated
| "express": "^4.13.3", | ||
| "fastboot": "^1.0.0-rc.0", | ||
| "fastboot-express-middleware": "^1.0.0-rc.3", | ||
| "fastboot-express-middleware": "dollarshaveclub/fastboot-express-middleware#b975bb3", |
There was a problem hiding this comment.
This should be updated to a released version before merge.
test/app-server-test.js
Outdated
| }); | ||
|
|
||
| it("executes preFastbootMiddlewares", function() { | ||
| return runServer('prefastboot-middleware-server') |
There was a problem hiding this comment.
We discussed at the meeting this week that the API for this should be to expose pre-FastBoot middleware and post-FastBoot middleware callbacks that yield the Express app itself. This is a more flexible API that is not limited to just middleware, and lets folks use the Express API they're already familiar with instead of learning something custom to the FastBoot App Server.
arjansingh
force-pushed
the
middleware-hooks
branch
from
e198817 to
ea0be49
Compare
|
|
||
| this.preFastbootMiddlewares(app); | ||
|
|
||
| if (this.gzip) { |
There was a problem hiding this comment.
Had to move this code to get the execution order right.
src/express-http-server.js
Outdated
| let username = this.username; | ||
| let password = this.password; | ||
|
|
||
| this.preFastbootMiddlewares(app); |
| res.send(); | ||
| } | ||
|
|
||
| var server = new FastBootAppServer({ |
There was a problem hiding this comment.
@tomdale this pattern would be how you would use the middleware hooks.
README.md
Outdated
|
|
||
| const server = FastBootAppServer({ | ||
| preFastbootMiddlewares: function (app) { app.use(modifyRequest); }, | ||
| postFastbootMiddlewares: function (app) { app.use(handleErrors); } |
There was a problem hiding this comment.
This is a little verbose, and I think middleware is an irregular plural, right? "Middlewares" sounds strange to me.
Maybe something like:
beforeMiddlewareafterMiddleware
?
There was a problem hiding this comment.
That should do it
arjansingh
force-pushed
the
middleware-hooks
branch
from
2c19e4e to
d5e5a80
Compare
|
This is awesome. Thank you @arjansingh! |
3 participants
I just thought I'd start the review.
Requires ember-fastboot/fastboot-express-middleware#11 before we can merge.