-
Notifications
You must be signed in to change notification settings - Fork 1.1k
feat(rest): allow static assets to be served by a rest server #1611
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,9 @@ import { | |
| import {RestBindings} from './keys'; | ||
| import {RequestContext} from './request-context'; | ||
| import * as express from 'express'; | ||
| import {ServeStaticOptions} from 'serve-static'; | ||
| import {PathParams} from 'express-serve-static-core'; | ||
| import * as pathToRegExp from 'path-to-regexp'; | ||
|
|
||
| const debug = require('debug')('loopback:rest:server'); | ||
|
|
||
|
|
@@ -131,6 +134,7 @@ export class RestServer extends Context implements Server, HttpServerLike { | |
| protected _httpServer: HttpServer | undefined; | ||
|
|
||
| protected _expressApp: express.Application; | ||
| protected _routerForStaticAssets: express.Router; | ||
|
|
||
| get listening(): boolean { | ||
| return this._httpServer ? this._httpServer.listening : false; | ||
|
|
@@ -196,6 +200,9 @@ export class RestServer extends Context implements Server, HttpServerLike { | |
| }; | ||
| this._expressApp.use(cors(corsOptions)); | ||
|
|
||
| // Place the assets router here before controllers | ||
| this._setupRouterForStaticAssets(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This can be moved to be inside the if statement below so we only instantiate this router if a static asset is being added.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I have it outside of the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But you are calling this function from the |
||
|
|
||
| // Mount our router & request handler | ||
| this._expressApp.use((req, res, next) => { | ||
| this._handleHttpRequest(req, res, options!).catch(next); | ||
|
|
@@ -209,6 +216,17 @@ export class RestServer extends Context implements Server, HttpServerLike { | |
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Set up an express router for all static assets so that middleware for | ||
| * all directories are invoked at the same phase | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please note that each new Router layer adds a non-negligible delay to the observed latency (time needed to handle the request) and also reduces the amount of requests that the server can handle every second. I don't understand what's the purpose of creating a new
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this should be mostly an implementation detail that we can easily change later. If you prefer, then I am ok to keep the currently proposed implementation for now.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's the need to support
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand that and wanted to proposed a different solution to that problem. Let's not waste too much time on this, I can later send a follow-up pull request to update the implementation to what I am envisioning. However, based on my other comment, we may need the extra |
||
| */ | ||
| protected _setupRouterForStaticAssets() { | ||
| if (!this._routerForStaticAssets) { | ||
| this._routerForStaticAssets = express.Router(); | ||
| this._expressApp.use(this._routerForStaticAssets); | ||
| } | ||
| } | ||
|
|
||
| protected _handleHttpRequest( | ||
| request: Request, | ||
| response: Response, | ||
|
|
@@ -513,6 +531,25 @@ export class RestServer extends Context implements Server, HttpServerLike { | |
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Mount static assets to the REST server. | ||
| * See https://expressjs.com/en/4x/api.html#express.static | ||
| * @param path The path(s) to serve the asset. | ||
| * See examples at https://expressjs.com/en/4x/api.html#path-examples | ||
| * To avoid performance penalty, `/` is not allowed for now. | ||
| * @param rootDir The root directory from which to serve static assets | ||
| * @param options Options for serve-static | ||
| */ | ||
| static(path: PathParams, rootDir: string, options?: ServeStaticOptions) { | ||
| const re = pathToRegExp(path, [], {end: false}); | ||
| if (re.test('/')) { | ||
| throw new Error( | ||
| 'Static assets cannot be mount to "/" to avoid performance penalty.', | ||
| ); | ||
| } | ||
| this._routerForStaticAssets.use(path, express.static(rootDir, options)); | ||
| } | ||
|
|
||
| /** | ||
| * Set the OpenAPI specification that defines the REST API schema for this | ||
| * server. All routes, parameter definitions and return types will be defined | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| <html> | ||
| <header> | ||
| <title>Test Page</title> | ||
| </header> | ||
| <body> | ||
| <h1>Hello, World!</h1> | ||
| </body> | ||
| </html> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❗️ The static router must be mounted AFTER dynamic routes. This is super important for performance. ❗️
In your current design, every REST API endpoint like
/products/123is going to hit the file system to check if there is a file matching the URL, e.g./html/products/123.Please add a test to verify what happens when there is both a LB4 route (e.g. a controller method) and a static file mapped at the same URL.
The correct order of middleware:
This is BTW the same design we have in LB 3.x, it took us few iterations and lot of learning until we figured it out correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's not true.
/products/123won't match/htmland it will be skipped without touching the file system.I'm fine with this order but the LB4 router does not pass control to
nextat the moment.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought express makes a map of the routes in memory of all the files mounted for static serving? Isn't that the point of the express Router?