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

raymondfeng · 2018-08-31T22:00:41Z

Depends on #1611

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

bajtos · 2018-09-03T08:25:05Z

I am not comfortable adding swagger-ui as a dependency of @loopback/rest:

Whenever swagger-ui make a breaking change, then we would have to release a new major version of @loopback/rest.
It's difficult for LB4 users to use their own fork of our explorer extension, because they have to fork @loopback/rest too.
Extensibility was one of the top-most design priorities for LB4. If we cannot implement @loopback/explorer as an independent component, then IMO we have failed at the extensibility goal.

Could you please look for other options on how to add local swagger-ui?

Ideally, I'd like @loopback/explorer to be a standard LB4 extension that can be added to an application as follows:

import {ExplorerComponent} from '@loopback/explorer';

class MyApp extends RestApplication {
  constructor() {
    app.component(ExplorerComponent);
  }
}

Implementation wise, we need the server to provide the following two features IIUC:

Serve static assets of swagger-ui. Ideally, I'd like us to build on the recently landed support for static assets (feat(rest): allow static assets to be served by a rest server #1611). I think the solution is to call app.static() from ExplorerComponent constructor?
Provide dynamic configuration for static swagger-ui front-end.

In LB 3.x, we solved this problem by adding server route GET /config.json (source) which is called by front-end setup code in loadSwaggerUI.js. (See also WIP(swagger-ui): Upgrade to v3 strongloop/loopback-component-explorer#209 which was upgrading LB3.x Explorer to use a newer version of swagger-ui.)

In LB4, we can implement GET /config.json as a regular route (or even a Controller method) - extensions have been designed for that! To hide this internal route from OpenAPI spec of the application, I am proposing to add a new OpenAPI metadata flag allowing developers to hide the endpoint from the public spec. For example, remoting metadata in LB 3.x has a flag called documented, see feat(route-helper): Add 'documented' flag for hiding params strongloop/loopback-swagger#107

It makes we wonder though: since the explorer is mounted on the app and served at the same scheme+hostname+port address, is this configuration needed at all? Cannot we use /openapi.json as the URL provided to swagger-ui, letting it up to the browser to resolve a relative URL path into a full absolute URL?

bajtos

Let's discuss the high-level design first, see my comment above.

hacksparrow · 2018-09-04T14:36:03Z

Yes, @loopback/explorer should be a LB4 extension. Baking it in with @loopback/rest doesn't sound right.

raymondfeng · 2018-09-04T15:07:25Z

@bajtos @hacksparrow I agree that it should be an extension to rest.

I'm just taking baby steps to get there. :-).

raymondfeng · 2018-09-04T20:17:20Z

I took a step to create ExplorerComponent, which contributes an Express middleware and expects to receive the configuration from the context.

There seems to be few gaps to have RestComponent to collaborate with ExplorerComponent. See FIXME sections on https://github.com/strongloop/loopback-next/blob/cba9e21efb4593c025df707b7c35514e77727379/packages/explorer/test/integration/rest.integration.ts. It boils down to how to divide responsibility between the two components.

virkt25 · 2018-09-05T18:07:49Z

packages/explorer/test/integration/rest.integration.ts

+    // FIXME: How do we configure the API Explorer UI?
+    server.bind(ExplorerBindings.CONFIG).to({});
+
+    // FIXME: Should the ExplorerComponent contribute a handler or route?


I think it might be better to contribute a route. Do we even support multiple handlers within an application?

How do we contribute a route?

call APIs on RestServer such as route(...)? This will create a dependency from explorer to `rest.

bind the Explorer route to rest.server.routes? The tricky thing is that we can have multiple REST server instances. Do we use inject.setter to bind it to the server context?

I guess this is a missing piece that needs to be figured out. I think ideally a component should be allowed to contribute a route ... similar to how it can contribute a component and then rest will figure out the mapping of the files to the route.

At the moment, extensions can contribute controller (classes). So instead of contributing a single route, Explorer can contribute controller class with a single method (HTTP endpoint). That should work OOTB right now, BUT this new endpoint will be included in OpenAPI spec documenting application's API. I think we should add an option for hiding certain endpoints from the documentation, e.g. @undocumented or @private.

call APIs on RestServer such as route(...)? This will create a dependency from explorer to `rest.

I am not really concerned about this. IMO, the API explorer cannot function with the REST component, because it's tightly coupled with OpenAPI.

If you are concerned about coupling at the level of Node.js dependencies, then I am proposing to define a minimal RestApplication interface inside the Explorer extension, this interface can describe the route API we need.

hacksparrow · 2018-09-06T10:06:05Z

Btw, what do we want the url to be? /swagger-ui or /api-explorer? /api-explorer sounds more intuitive to me, besides being the same as in LB3.

bajtos · 2018-09-07T14:09:55Z

packages/rest/src/rest.server.ts

-      // E.g. if the app implements access/audit logs, I don't want
-      // this endpoint to trigger a log entry. If the server implements
-      // content-negotiation to support XML clients, I don't want the OpenAPI
-      // spec to be converted into an XML response.


Please preserve this comment.

bajtos · 2018-09-07T14:12:12Z

packages/rest/src/rest.server.ts

+    // Serving OpenAPI spec
+    for (const p in mapping) {
+      this._expressApp.use(p, (req, res) =>
+        this._serveOpenApiSpec(req, res, mapping[p]),


First of all, please move this new setup code to a new method, similar to _setupRouterForStaticAssets.

Secondly, I think this change makes sense on its own and should have been made as part of #1637. Could you please move it to #1637 or perhaps open a new PR after #1637 is landed? I'd like to get this cleanup landed quickly and fear that the Explorer pull request will require longer to get into a mergeable state.

bajtos · 2018-09-07T14:15:34Z

packages/explorer/test/integration/rest.integration.ts

+    // FIXME: How to register routes to the rest server by phase or order
+    // FIXME: Should REST server automatically mounts the route based on registration
+    // via bindings such as `rest.server.routes`?
+    server.staticRouter.use('/explorer', handler);


I find this very hacky.

Just the fact that staticRouter is a public property is alarming to me, as it opens door to all kinds of hacks that people can start making with our internal implementation details of app.static(). Let's fix that please and make server.staticRouter a private property.

Secondly, I thought the entire purpose of app.static was to allow the explorer component to serve swagger-ui static assets. I don't fully understand why you don't want to use it?

IMO, we should focus on RestApplication scenario in the first iteration because multi-server apps are not officially supported yet, and call app.static() in component constructor.

Thoughts?

bajtos · 2018-09-07T14:16:07Z

packages/rest/src/rest.server.ts

@@ -561,6 +555,10 @@ export class RestServer extends Context implements Server, HttpServerLike {
    this._routerForStaticAssets.use(path, express.static(rootDir, options));
  }

+  get staticRouter() {


Please don't, see my comment above.

I agree. I'm playing with various options here to surface the problems.

bajtos · 2018-09-07T14:17:40Z

packages/explorer/test/integration/rest.integration.ts

+async function givenAServer(options?: {rest: RestServerConfig}) {
+  const app = new Application(options);
+  app.component(RestComponent);
+  // FIXME: Can we mount the ExplorerComponent to a given server?


I think this is a missing extension point?

At the moment, components can customize only the entire Application, they don't have a good access to individual Server instances.

However, for 4.0 GA, we support only single-server applications using RestApplication, so I would not worry about this too much. I am proposing to create a follow-up issue for post-GA and move on.

bajtos · 2018-09-07T14:23:45Z

I'd like to propose an entirely different approach.

IMO, LoopBack is primarily intended for REST API servers, it's not a universal HTTP server good for serving single-page applications. API Explorer is a single page application.

What if we leverage vanilla express to build API Explorer and serve the explorer on a different port?Or perhaps use middleware composition to add LoopBack application as a middleware in the top-level express app?

const apiApp = new TodoApplication();
const container = express();

container.use('/api', apiApp.requestHandler);
container.use('/explorer-config', (req, res) => {
  // serve dynamic configuration needed by swagger-ui
  // see loopback-component-explorer for details
});
container.use('/explorer', express.static('path-to-swagger-ui'));

Thoughts?

hacksparrow · 2018-09-07T14:34:50Z

I'd like to propose an entirely different approach.

I like that approach - less disruptive and simpler to implement.

Things to consider:

Make Explorer optional
Maybe run on a separate process?

virkt25 · 2018-09-11T23:41:21Z

@raymondfeng Can you please rebase this PR? Something seems off as I'm seeing 28 commits & 105 file changes and can't do a review at this time as a result.

raymondfeng · 2018-09-17T22:08:13Z

@bajtos @virkt25 I reworked the PR to allow explorer component to contribute a middleware to the REST server. PTAL.

bajtos · 2018-09-18T11:53:21Z

I am not happy at all about allowing third party components and user applications to access the internal Express router.

Ensuring the correct order of middleware registration has been always a difficult problem. We have somewhat improved the situation by using middleware phases in LoopBack 3.x, but it was far from perfect.
One of the main design goals of LoopBack Next was to abandon the concept of Express(-like) middleware and use better design for composing different request processing pieces.
By exposing the built-in express router, we are putting ourselves into a corner. It will be difficult to replace Express with a better solution in the future.

In one of my earlier comments, I was proposing to use app.static() API that was added primarily to support API Explorer use case.

Secondly, I thought the entire purpose of app.static was to allow the explorer component to serve swagger-ui static assets. I don't fully understand why you don't want to use it?

IMO, we should focus on RestApplication scenario in the first iteration because multi-server apps are not officially supported yet, and call app.static() in component constructor.

At the moment, components can customize only the entire Application, they don't have a good access to individual Server instances.

However, for 4.0 GA, we support only single-server applications using RestApplication, so I would not worry about this too much. I am proposing to create a follow-up issue for post-GA and move on.

I am not entirely against using a different approach, but need to hear an explanation why we cannot use app.static() in the first place - what problems would that introduce?

Considering that an explorer hosted at explorer.loopback.io is good enough for 4.0 GA, and the pull request loopbackio/explorer.loopback.io#1 should hopefully land in few days time, I think we should avoid shortcuts in this pull request and aim for a well-designed solution.

For example:

Use app.static to serve static swagger-ui assets
Define a new flag (OpenAPI OperationObject extension) that will mark certain HTTP endpoints as hidden from the documentation (the OpenAPI spec generated for the app).
Define a hidden route (either via app.route or by exporting a controller from the component) that will return JSON file with the dynamic configuration needed by swagger-ui, see how loopback-component-explorer works. Use the new flag (see item 2) to hide this route from API spec & docs.

raymondfeng · 2018-09-18T14:58:22Z

I am not entirely against using a different approach, but need to hear an explanation why we cannot use app.static() in the first place - what problems would that introduce?

Please note we allow customization of the home page and render it as an EJS view while keeping relative paths to js assets from swagger-ui-dist. app.static() does not support views.

Considering that an explorer hosted at explorer.loopback.io is good enough for 4.0 GA, and the pull request loopbackio/explorer.loopback.io#1 should hopefully land in few days time, I think we should avoid shortcuts in this pull request and aim for a well-designed solution.

Sure, explorer.loopback.io will buy us some buffer so that we can flush out a reasonable approach.

raymondfeng · 2018-09-18T15:11:57Z

I am not happy at all about allowing third party components and user applications to access the internal Express router.

In the latest code, I have removed the exposure of the internal Express router. Instead, we allow extensions to bind an Express middleware to the extension point.

Ensuring the correct order of middleware registration has been always a difficult problem. We have somewhat improved the situation by using middleware phases in LoopBack 3.x, but it was far from perfect.

#1671 is my experiment to address the ordering issue.

It adds phase-based relative ordering to register handlers
It allows (ctx, chain) => Promise style handler, which has much better control for one-way (express style) or cascading (chain.next(), Koa style). Also special $final and $error phases are added to support try-catch-finally.
It allows plug-in of existing Express middleware as-is, but wrap them internally to be consistent with our new handler design/signature.

One of the main design goals of LoopBack Next was to abandon the concept of Express(-like) middleware and use better design for composing different request processing pieces.

Yes and No. Yes - We want to have a better handler/chain design. No - One of the major reason of re-adopting Express as the http container is to allow the pluggability of existing Express middleware modules as-is.

By exposing the built-in express router, we are putting ourselves into a corner. It will be difficult to replace Express with a better solution in the future.

Again, we don't expose Router directly. Instead, we'll offer an API such as server.middleware to register Express middleware modules. The Express routing is not necessarily used behind the scene. Please also note that Express Router is different from its routing. I see a Router as a grouping of middleware handlers.

We should open a new issue to discuss how to position Express with LoopBack 4 and what's the ideal programming model to get the best of both breeds.

virkt25 · 2018-09-19T14:44:34Z

packages/rest/src/rest.server.ts

@@ -272,13 +282,29 @@ export class RestServer extends Context implements Server, HttpServerLike {
    return this.httpHandler.handleRequest(request, response);
  }

+  protected _registerMiddleware() {


virkt25 · 2018-09-19T14:45:40Z

packages/rest/src/rest.server.ts

+      credentials: true,
+    };
+    // Set up CORS
+    this._expressApp.use(cors(corsOptions));


Can we register this using this._registerMiddlerware() to serve as an example.

bajtos · 2018-11-12T09:31:23Z

I opened a new PR inspired by this work, see #2014

raymondfeng requested a review from bajtos as a code owner August 31, 2018 22:00

raymondfeng changed the base branch from master to allow-static-assets August 31, 2018 22:01

raymondfeng changed the title ~~[WIP]RFC] Add local swagger ui for LoopBack 4 API explorer~~ [WIP][RFC] Add local swagger ui for LoopBack 4 API explorer Aug 31, 2018

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

raymondfeng force-pushed the local-swagger-ui branch from 80c4a4a to 4b58087 Compare August 31, 2018 22:39

raymondfeng changed the base branch from allow-static-assets to master August 31, 2018 22:39

raymondfeng force-pushed the local-swagger-ui branch 2 times, most recently from 8e7097c to dfda1d8 Compare September 1, 2018 04:49

raymondfeng changed the base branch from master to fix-openapi-server-url September 1, 2018 20:54

raymondfeng force-pushed the local-swagger-ui branch from dfda1d8 to e526609 Compare September 1, 2018 21:00

bajtos suggested changes Sep 3, 2018

View reviewed changes

raymondfeng force-pushed the local-swagger-ui branch from e526609 to 7291ead Compare September 4, 2018 19:17

raymondfeng force-pushed the fix-openapi-server-url branch 2 times, most recently from 828295d to 154f434 Compare September 4, 2018 19:53

raymondfeng force-pushed the local-swagger-ui branch from 7291ead to cba9e21 Compare September 4, 2018 20:13

virkt25 reviewed Sep 5, 2018

View reviewed changes

raymondfeng force-pushed the fix-openapi-server-url branch from 154f434 to d2572f3 Compare September 5, 2018 21:47

bajtos reviewed Sep 7, 2018

View reviewed changes

raymondfeng force-pushed the fix-openapi-server-url branch 2 times, most recently from b56fb37 to eca79a4 Compare September 7, 2018 16:36

This was referenced Sep 7, 2018

feat: host swagger UI at v4.loopback.io/api-explorer strongloop/v4.loopback.io#34

Closed

feat(rest): provide more options to customize api explorer and openapi spec #1637

Merged

raymondfeng force-pushed the local-swagger-ui branch from cba9e21 to d5e9db2 Compare September 7, 2018 16:48

raymondfeng requested review from jannyHou, marioestradarosa and shimks as code owners September 11, 2018 22:00

raymondfeng changed the base branch from fix-openapi-server-url to master September 12, 2018 14:48

raymondfeng force-pushed the local-swagger-ui branch 3 times, most recently from 2068c9b to b2ca7dc Compare September 17, 2018 21:16

raymondfeng self-assigned this Sep 18, 2018

raymondfeng added post-GA API Explorer labels Sep 18, 2018

virkt25 reviewed Sep 19, 2018

View reviewed changes

raymondfeng mentioned this pull request Oct 24, 2018

[API Explorer] Self-hosted API Explorer #559

Closed

2 tasks

feat(explorer): add a package for local API Explorer UI

bf10d3a

raymondfeng force-pushed the local-swagger-ui branch 2 times, most recently from bb71f55 to 2e9730e Compare October 31, 2018 21:11

feat(rest): add local UI for api explorer

c1d710b

raymondfeng force-pushed the local-swagger-ui branch from 2e9730e to c1d710b Compare November 1, 2018 15:20

raymondfeng mentioned this pull request Nov 1, 2018

[WIP] feat: local API Explorer #1957

Closed

7 tasks

dhmlau removed the post-GA label Nov 2, 2018

bajtos mentioned this pull request Nov 12, 2018

A self-hosted API Explorer as a new extension #2014

Merged

7 tasks

bajtos mentioned this pull request Nov 12, 2018

feat(rest): add config option to disable API Explorer redirects #2016

Merged

4 tasks

apocaliss92 approved these changes Nov 15, 2018

View reviewed changes

bajtos closed this in #2014 Nov 15, 2018

bajtos deleted the local-swagger-ui branch November 19, 2018 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

raymondfeng commented Aug 31, 2018 •

edited

Loading

bajtos commented Sep 3, 2018

bajtos left a comment

hacksparrow commented Sep 4, 2018

raymondfeng commented Sep 4, 2018

raymondfeng commented Sep 4, 2018 •

edited

Loading

virkt25 Sep 5, 2018

raymondfeng Sep 5, 2018

virkt25 Sep 11, 2018

bajtos Sep 13, 2018

bajtos Sep 13, 2018

hacksparrow commented Sep 6, 2018

bajtos Sep 7, 2018

bajtos Sep 7, 2018

bajtos Sep 7, 2018

bajtos Sep 7, 2018

raymondfeng Sep 12, 2018

bajtos Sep 7, 2018

bajtos commented Sep 7, 2018

hacksparrow commented Sep 7, 2018

virkt25 commented Sep 11, 2018

raymondfeng commented Sep 17, 2018

bajtos commented Sep 18, 2018

raymondfeng commented Sep 18, 2018

raymondfeng commented Sep 18, 2018

virkt25 Sep 19, 2018

virkt25 Sep 19, 2018

bajtos commented Nov 12, 2018

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

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

Conversation

raymondfeng commented Aug 31, 2018 • edited Loading

Checklist

bajtos commented Sep 3, 2018

bajtos left a comment

Choose a reason for hiding this comment

hacksparrow commented Sep 4, 2018

raymondfeng commented Sep 4, 2018

raymondfeng commented Sep 4, 2018 • 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

hacksparrow commented Sep 6, 2018

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 Sep 7, 2018

hacksparrow commented Sep 7, 2018

virkt25 commented Sep 11, 2018

raymondfeng commented Sep 17, 2018

bajtos commented Sep 18, 2018

raymondfeng commented Sep 18, 2018

raymondfeng commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Nov 12, 2018

raymondfeng commented Aug 31, 2018 •

edited

Loading

raymondfeng commented Sep 4, 2018 •

edited

Loading