feat(rest): allow static assets to be served by a rest server #1611

raymondfeng · 2018-08-15T16:18:03Z

This PR allows static assets to be served from a REST server by calling static().

Missing tests identified by @bajtos

What happens when there is both a LB4 route (e.g. a controller method) and a static file mapped at the same URL.
New static assets can be registered after the app was started.
~~When a file cannot be read (e.g. because of permission problems), the error response is sent by the Sequence (as provided/customized by the app).~~

Checklist

npm test passes on your machine
New tests added or existing tests modified to cover all changes
Code conforms with the style guide
API Documentation in code was updated
Documentation in /docs/site was updated
Affected artifact templates in packages/cli were updated
Affected example projects in examples/* were updated

virkt25

YAY!! 💯 🥇 👍 🙇

Much needed feature IMO! I also love how simple the implementation is.

Just needs some documentation -- in the package README + loopback.io docs and this is good to land. Maybe a quick blog, if not we can include it in the monthly milestone blog.

virkt25 · 2018-08-15T17:57:06Z

Related to #691

Based on the issue, what are your thoughts on introducing a sugar method app.static(path, {options})? And a test around missing files /html/not-a-real-file.html to see what error we return / documenting what a user can expect?

raymondfeng · 2018-08-15T19:27:50Z

@virkt25 Added more tests and an .asset() api to register static routes by code.

rmg · 2018-08-15T20:36:15Z

I don't see serve-static in the dependencies listed in package.json. Shouldn't it be?

raymondfeng · 2018-08-15T20:37:26Z

@rmg serve-static is a hard dependency of express at the moment. We only use it for typing. Maybe a dependency of @types/serve-static is better off.

rmg · 2018-08-15T20:38:39Z

Shouldn't it still be listed as a dependency then? Otherwise you're relying on dependency flattening for this to work.

raymondfeng · 2018-08-15T20:46:22Z

@rmg We only use serve-static to reference ServeStaticOptions type for TypeScript compilation. Unfortunately express does not expose it. Adding an explicit dependency might cause conflict against the bundled version of Express.

virkt25 · 2018-08-15T20:46:48Z

packages/rest/src/rest.server.ts

+   * @param options Options for serve-static
+   */
+  asset(path: string, root: string, options?: ServeStaticOptions) {
+    this._setupAssetsRouter();


Should we use the same static router for all static files because right now each folder would get it's own.

No, we have the same router - this._assetsRouter. It's the container for multiple middleware, each per root dir.

virkt25 · 2018-08-15T20:48:40Z

packages/rest/src/rest.application.ts

+   * @param root The root directory
+   * @param options Options for serve-static
+   */
+  asset(path: string, root: string, options?: ServeStaticOptions) {


Can we rename this to static -- it's traditional and what developers would expect. Asset to me can imply anything ... a controller can be an asset imo.

rmg · 2018-08-15T20:55:09Z

Oh, I see how it is being used now.. I didn't notice the middleware was used as express.static and the import was only being used for typing.

raymondfeng · 2018-08-15T21:01:53Z

@virkt25 Made some name changes.

virkt25

LGTM. One last comment + needs docs (if that’ll be a follow up PR then just the one last comment).

virkt25 · 2018-08-15T21:37:43Z

packages/rest/src/rest.server.ts

@@ -196,6 +198,21 @@ export class RestServer extends Context implements Server, HttpServerLike {
    };
    this._expressApp.use(cors(corsOptions));

+    // Place the assets router here before controllers
+    this._setupRouterForStaticAssets();


This can be moved to be inside the if statement below so we only instantiate this router if a static asset is being added.

No, I have it outside of the if statement on purpose as we need to register the middleware slot so that .static() will work.

But you are calling this function from the .static() method as well ... so maybe remove it from there?

raymondfeng · 2018-08-15T21:59:58Z

@virkt25 Added some docs.

virkt25

LGTM once the last comment is addressed.

virkt25 · 2018-08-15T22:51:58Z

packages/rest/src/rest.server.ts

+   * @param options Options for serve-static
+   */
+  static(path: string, root: string, options?: ServeStaticOptions) {
+    this._setupRouterForStaticAssets();


This isn’t needed because of the line above.

bajtos

Serving static files is tricky, let's make sure we get it right.

Besides the comments below, please add a test to verify that new static assets can be registered after the app was started.

await createClientForHandler(server.requestHandler)
  .get('/html/index.html')
  .expect(404);

server.static('/html', /*...*/);

await createClientForHandler(server.requestHandler)
  .get('/html/index.html')
  .expect(200);

bajtos · 2018-08-23T15:24:45Z

packages/rest/src/rest.server.ts

@@ -196,6 +198,21 @@ export class RestServer extends Context implements Server, HttpServerLike {
    };
    this._expressApp.use(cors(corsOptions));

+    // Place the assets router here before controllers


❗️ 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/123 is 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:

LB4 router

static files

error handler

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.

In your current design, every REST API endpoint like /products/123 is going to hit the file system to check if there is a file matching the URL, e.g. /html/products/123.

That's not true. /products/123 won't match /html and it will be skipped without touching the file system.

The correct order of middleware:
LB4 router
static files
error handler

I'm fine with this order but the LB4 router does not pass control to next at the moment.

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?

bajtos · 2018-08-23T15:29:30Z

packages/rest/src/rest.server.ts

@@ -209,6 +226,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


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 express.Router() dedicated to serving static assets? Is this needed to support app.static API? If it is, then I am proposing to use a different implementation where we collect all information provided by app.static (e.g. list of arguments passed to every invocation) and then invoke static middleware factory only when constructing the ultimate request handler.

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.

It's the need to support app.static() api. This is the same express problem that we worked around in LB3 with middleware phases.

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 express.Router instance to allow sequence-based implementation, in which case this discussion is not relevant.

bajtos · 2018-08-24T07:33:50Z

Let's move the discussion to the main thread so that it does not get hidden when you change the code.

That's not true. /products/123 won't match /html and it will be skipped without touching the file system.

Ah, you are right. I agree that when the static middleware is mounted at a path that's different from API endpoints, then the order of the LB4 router vs. serve-static does not matter.

Should we look for a way how to ensure that static middleware is always mounted on a non-root path that prevents conflicts with route/controller-based paths?

My concern is that people will use this feature to build single-page applications and mount the static middleware at the root, to be able to serve http://localhost:3000/index.html file.

I'm fine with this order but the LB4 router does not pass control to next at the moment.

Hmm, this is actually more tricky than it looks at the first sight.

In LB4, API requests are handled by a Sequence, which is designed with the assumption that the Sequence is going to handle all requests and implement error handling too (including 404 not found).

IMO, removing this assumption and changing the design of Sequence to add a third possible outcome - let somebody else handle the request - would be a big step backwards and we should avoid that.

Also mounting serve-static middleware at Express layer is bypassing the sequence entirely. This is likely to have unintended consequences that may bite us hard. For example, happens serve-static cannot read a file, e.g. because of a permission problem, then it forward the error via next(err) for the error-handling middleware to take care of that (see here). The problem is that we don't have a way how to invoke the reject sequence action to handle that error! In fact, the sequence may not even use reject and can handle errors differently.

I am wondering: Can we define a special kind of a catch-all route that will:

match any URL that did not match any API endpoint; i.e. the sequence action findRoute returns this catch-all route if no API endpoint matched the requested URL
run the express Router where static assets were mounted; i.e. the sequence action invoke runs express routing to handle static assets
throw HttpError.NotFound when no static asset was found, i.e. the sequence action invoke throws the 404 error

In my mind, such solution would solve most if not all issues:

static middleware is invoked after any routes
static middleware is invoked as part of the sequence
404 errors are still handled by the sequence

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?

It does not.

For example, in the development mode, you want the serve-static middleware to pick up any new files you added to your application (e.g. new front-end assets) without having to restart the applicaton.

Another example - a file storage service allowing clients to upload new assets that are later publicly available for download.

I agree it would be great if there was an option how to scan existing files and build an in-memory map of static-file routes, unfortunately serve-static does not support this mode.

raymondfeng · 2018-08-24T14:22:20Z

I think a final route/phase makes sense. By default, it will report 404.

For both static and final, the implementation can be improved as follows:

function staticMiddleware(ctx, staticFileMap) {
  let staticRouter = ctx.getSync('express.routers.static', {optional: true});
  if (staticRouter === undefined) {
    staticRouter = express.Router();
    // Mount initial set of static files to the router
    for(const p in staticFileMap) {
      staticRouter.use(p, ...);
    }
    ctx.bind('express.routers.static').to(staticRouter);
  } 
  return (req, res, next) => {
    // const staticRouter = ctx.getSync('express.routers.static');
    staticRouter(req, res, next);
  }
}

The static router maintains a map of (path, root) for static files and it can be dynamically updated without impacting the express middleware chain.

raymondfeng · 2018-08-26T20:10:40Z

BTW, I think we should refine the solution iteratively. For now, let's make it possible to serve static files. In the longer term, we should come up a better design that allow Express middleware and LB4 Sequence of actions to be friendly to each other.

bajtos · 2018-08-30T12:25:27Z

BTW, I think we should refine the solution iteratively. For now, let's make it possible to serve static files. In the longer term, we should come up a better design that allow Express middleware and LB4 Sequence of actions to be friendly to each other.

Sure. I have two main concerns about the current proposal.

Performance. As I commented in feat(rest): allow static assets to be served by a rest server #1611 (comment), when the user mounts a static middleware at / path (as is needed to serve /index.html for single-page applications), the current solution is going to hit the filesystem for every API request. IMO, that's not acceptable.

If it's too difficult to mount static middleware after REST API routes, then I am proposing to reject requests to mount static assets at the root path (/), to prevent users from unintentionally shooting into their own foot. (Don't forget to include a test.) Alternatively: instead of throwing an error, print a warning to console and include a link to a GitHub issue tracking the follow-up work.
Error handling. In the current proposal, the when a static asset was not found and/or could not be read, the reject error handler is not invoked and a very bare-bone fallback handler is used instead.

https://github.com/strongloop/loopback-next/blob/0364b59409ce3691babf5f17b5cd444b4e7f72aa/packages/rest/src/rest.server.ts#L654-L666

I am concerned about the confusion this can cause to our users. If you think it's acceptable for the first iteration, then I can live with it as long as it's clearly documented.

Please add the following tests:

What happens when there is both a LB4 route (e.g. a controller method) and a static file mapped at the same URL.
New static assets can be registered after the app was started. It is not necessary to implement this feature right now, but it would be great to have a test describing the actual behavior, whatever it is going to be.

bajtos · 2018-08-30T12:32:08Z

New static assets can be registered after the app was started. It is not necessary to implement this feature right now, but it would be great to have a test describing the actual behavior, whatever it is going to be.

I see this is already covered by tests 👍 please ignore this part of my previous comment.

bajtos

Few more comments in addition to what I wrote above (#1611 (comment)).

bajtos · 2018-08-30T12:27:53Z

packages/rest/src/rest.application.ts

+   * @param root The root directory
+   * @param options Options for serve-static
+   */
+  static(path: string, root: string, options?: ServeStaticOptions) {


I find the names path and root too similar; it's too easy to confuse which one is for the URL path and which one is for the filesystem path. I am proposing the following renames:

path -> url, urlPath or endpointPath

root -> rootDir, fileRoot or serveFrom

Please update RestServer#static in the same way too.

bajtos · 2018-08-30T12:30:29Z

packages/rest/src/rest.server.ts

@@ -670,6 +708,15 @@ export interface RestServerOptions {
  cors?: cors.CorsOptions;
  apiExplorerUrl?: string;
  sequence?: Constructor<SequenceHandler>;
+  /**
+   * Static assets to be served using `express.static()`


See #742.

The ApplicationConfig class should contain only things like HTTP port and database connection strings, there should be no entries related to what component, controller, repositories (etc.) the application is using. User application classes should call this.controller(), this.repository() (etc.) to register their artifacts, and eventually use the new bootstrapper module.

Please remove this config option in favor of app.static API.

raymondfeng · 2018-08-30T18:07:19Z

@bajtos Most of your comments have been addressed. PTAL.

bajtos

Awesome, almost there.

Besides the comment below, could you please add the following test?

What happens when there is both a LB4 route (e.g. a controller method) and a static file mapped at the same URL.

Unless you want to keep this behavior as undefined to allow us to change it within a semver-minor version, in which case I'd like to see this undefined behavior clearly spelled out in docs/site/Application.md.

If you can get somebody else from the team to approve this pull request after you address my two comments, then feel free to proceed with landing without waiting for another review from me.

bajtos · 2018-08-31T14:16:08Z

packages/rest/src/rest.server.ts

+      path === '/' || // path is '/'
+      // '' or '/' is included in the array
+      (Array.isArray(path) && (path.includes('/') || path.includes('')))
+    ) {


Please note that path can be also a regular expression.

I am proposing to reject all Regexp-based path values. In the future, I'd like to handle as much routing as possible using a fast trie-based router and trie-based routing does not support arbitrary regexp routes. If we allow regexp routes now, then the change to a trie-based router would be backwards incompatible and would have to wait for a next semver-major release.

If you agree to reject regexp values, then please update tsdoc comments and the content in docs/site/Application.md too.

Please note that string paths can allow regex patterns. I'll use path-to-regexp to check if path matches /.

raymondfeng · 2018-08-31T16:09:08Z

@bajtos I improved the code and tests per your comment.

bajtos · 2018-10-01T07:19:20Z

Follow-up stories to address missing pieces:

Error handling for non-API routes Error handling for non-API routes #1784
Invoke serve-static after API router Invoke serve-static after API router #1785

raymondfeng requested a review from bajtos as a code owner August 15, 2018 16:18

raymondfeng mentioned this pull request Aug 15, 2018

API Explorer does not work in Safari (mixed HTTP/HTTPS content problem) #1603

Closed

6 tasks

virkt25 reviewed Aug 15, 2018

View reviewed changes

raymondfeng force-pushed the allow-static-assets branch 2 times, most recently from 3fab97d to b0c4f80 Compare August 15, 2018 19:26

raymondfeng force-pushed the allow-static-assets branch from b0c4f80 to e04f2da Compare August 15, 2018 19:51

virkt25 reviewed Aug 15, 2018

View reviewed changes

raymondfeng force-pushed the allow-static-assets branch from e04f2da to b80287b Compare August 15, 2018 21:01

virkt25 reviewed Aug 15, 2018

View reviewed changes

raymondfeng force-pushed the allow-static-assets branch from b80287b to 736071d Compare August 15, 2018 21:59

virkt25 approved these changes Aug 15, 2018

View reviewed changes

raymondfeng force-pushed the allow-static-assets branch 3 times, most recently from b54eccc to 038d99c Compare August 21, 2018 16:31

bajtos suggested changes Aug 23, 2018

View reviewed changes

bajtos assigned raymondfeng Aug 24, 2018

bajtos mentioned this pull request Aug 24, 2018

[API Explorer] - Add static file serving capabilities to RestServer #691

Closed

3 tasks

raymondfeng force-pushed the allow-static-assets branch from 038d99c to e41579d Compare August 26, 2018 18:03

raymondfeng force-pushed the allow-static-assets branch 2 times, most recently from cd9a78c to ae72ebc Compare August 27, 2018 22:43

bajtos suggested changes Aug 30, 2018

View reviewed changes

raymondfeng changed the title ~~[RFC] feat(rest): allow static assets to be served by a rest server~~ feat(rest): allow static assets to be served by a rest server Aug 30, 2018

raymondfeng force-pushed the allow-static-assets branch from ae72ebc to c9317a0 Compare August 30, 2018 18:05

bajtos reviewed Aug 31, 2018

View reviewed changes

bajtos approved these changes Aug 31, 2018

View reviewed changes

raymondfeng force-pushed the allow-static-assets branch from c9317a0 to dc80a46 Compare August 31, 2018 16:08

raymondfeng added 2 commits August 31, 2018 09:10

fix(docs): fix typo

0de6afa

fix(docs): fix typo in Repositories.md

8ddf793

raymondfeng force-pushed the allow-static-assets branch from dc80a46 to 11d970e Compare August 31, 2018 16:10

raymondfeng mentioned this pull request Aug 31, 2018

[WIP][RFC] Add local swagger ui for LoopBack 4 API explorer #1664

Closed

7 tasks

feat(rest): allow static assets to be served by a rest server

2fb8a59

raymondfeng force-pushed the allow-static-assets branch from 11d970e to 2fb8a59 Compare August 31, 2018 22:03

raymondfeng merged commit a1cefcc into master Aug 31, 2018

raymondfeng deleted the allow-static-assets branch August 31, 2018 22:39

This was referenced Oct 1, 2018

Error handling for non-API routes #1784

Closed

Invoke serve-static after API router #1785

Closed

feat(rest): allow static assets to be served by a rest server #1611

feat(rest): allow static assets to be served by a rest server #1611

Conversation

raymondfeng commented Aug 15, 2018 • edited by bajtos Loading

Missing tests identified by @bajtos

Checklist

virkt25 left a comment

Choose a reason for hiding this comment

virkt25 commented Aug 15, 2018

raymondfeng commented Aug 15, 2018

rmg commented Aug 15, 2018

raymondfeng commented Aug 15, 2018 • edited Loading

rmg commented Aug 15, 2018

raymondfeng commented Aug 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmg commented Aug 15, 2018

raymondfeng commented Aug 15, 2018

virkt25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Aug 15, 2018

virkt25 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Aug 24, 2018

raymondfeng commented Aug 24, 2018 • edited Loading

raymondfeng commented Aug 26, 2018

bajtos commented Aug 30, 2018

bajtos commented Aug 30, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Aug 30, 2018

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Aug 31, 2018

bajtos commented Oct 1, 2018

raymondfeng commented Aug 15, 2018 •

edited by bajtos

Loading

raymondfeng commented Aug 15, 2018 •

edited

Loading

bajtos left a comment •

edited

Loading

raymondfeng commented Aug 24, 2018 •

edited

Loading