[WIP] feat: local API Explorer #1957

hacksparrow · 2018-11-01T13:05:02Z

Add local API Explorer

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

Add local API Explorer

bajtos · 2018-11-01T13:06:31Z

packages/core/src/component.ts

+/**
+ * A map of name/class pairs for binding static assets
+ */
+export type staticAssetsMap = {


Uff, then name and comment seems very misleading to me, because there is no map involved AFAICT.

Also please use interface instead of type.

bajtos · 2018-11-01T13:08:10Z

packages/rest/src/explorer.component.ts

+  staticAssets = [
+    {
+      path: '/explorer1',
+      rootDir: './public',


How is this relative path going to be resolved into an absolute one? IMO, it should be the responsibility of the component to resolve the path. E.g. rootDir: path.resolve(__dirname, '../../public')

bajtos · 2018-11-01T13:10:19Z

packages/rest/src/rest.application.ts

 import {format} from 'util';
 import {RestBindings} from './keys';
 import {RestComponent} from './rest.component';
+import {ExplorerComponent} from './explorer.component';


Don't bundle the explorer component inside @loopback/rest. The coupling between the API explorer (e.g. a specific swagger-ui version used) and the runtime (@loopback/rest) is the thing we want to avoid by moving explorer into a new component.

The API Explorer should be added to applications as part of our project scaffolding provided by lb4 app. Nice to have: lb4 app can ask the user whether they want to add API Explorer to their project or not.

bajtos · 2018-11-01T13:12:51Z

packages/rest/src/rest.application.ts

+      mountComponent(this, explorerInstance);
+      console.log(explorerInstance.staticAssets);
+      const explorer = explorerInstance.staticAssets![0];
+      this.static(explorer.path, explorer.rootDir, explorer.options);


This block needs to be split into two parts:

RestApplication class should override component method provided by Application, iterate over component.staticAssets and call this.static to register them.

The scaffolded project-specific application class should register the component in the constructor, e.g.

class TodoApplication extends RepositoryMixin(BootMixin(RestApplication)) { constructor () { // ... this.component(ExplorerComponent); } }

bajtos · 2018-11-01T13:13:41Z

packages/core/src/component.ts

+  /**
+   * An array of directory to serve static assets
+   */
+  staticAssets?: staticAssetsSettings[];


I think these REST-specific extensions should be defined in a new interface inside @loopback/rest, because applications using different transports (e.g GraphQL or gRPC) cannot support static assets.

Should we generalize it to allow components to contribute middleware based routes?

Sounds like a separate feature to me.

+1 to leave middleware-based routes out of scope.

Personally, I would like to avoid middleware-based routes for as long as we can. LB4 already supports handler-based routes that play nice with OpenAPI, I would prefer extensions to use that approach.

bajtos · 2018-11-01T13:14:57Z

packages/core/src/component.ts

+/**
+ * Static assets settings
+ */
+export type staticAssetsSettings = {


export interface StaticAssetsSettings (use interface, not type; interface names start with a capital letter)

I would also pick a slightly different name, e.g. staticAssetsDefinition or staticAssetsConfig.

review 1

raymondfeng · 2018-11-01T15:21:22Z

@hacksparrow Are you proposing it as a replacement to #1664?

raymondfeng · 2018-11-01T17:04:03Z

packages/rest/src/types.ts

+  options?: object;
+}
+
+export interface Component extends CoreComponent {


It's probably better to name it as ComponentWithStaticAssets.

We my want to add additional exports besides static assets later in the future. I think RestComponent would be a better name?

I also think the interface should not be inheriting from CoreComponent to allow composition of multiple transport-specific interfaces, e.g. Component & RestComponent & GrapQLComponent.

Don't we have a class named as RestComponent?

Don't we have a class named as RestComponent
@raymondfeng Yes, we have that already. For now I have renamed it to StaticComponent.

I also think the interface should not be inheriting from CoreComponent ...

@bajtos in that case we'll have to do componentCtor: Constructor<Component | StaticComponent>, etc. Is that OK?

I think this should be an independent interface from RestComponent, StaticComponent does not need any of the properties defined in RestComponent. Clubbing them together is not efficient.

Don't we have a class named as RestComponent?

We actually do! It's a component adding REST servers to an app.

The intention here was to describe a component that's contributing REST-specific artifacts like static assets.

What a confusion 🙈

raymondfeng · 2018-11-01T17:05:39Z

packages/rest/src/rest.application.ts

    this.component(RestComponent);
  }

+  component(componentCtor: Constructor<Component>, name?: string) {


What if an application does not extend from RestApplication but mount the RestComponent?

If the app is extending from Application, then there is no REST server configured by default and the rest-specific exports are silently ignored.

What I'm trying to say is that RestComponent should be responsible for mounting such routes. In this case, other components contribute routes for the RestComponent.

@raymondfeng can you elaborate on the above statement, please?

A developer can do the the following:

const app = new Application(); app.component(RestComponent); app.component(ExplorerComponent);

With the current implementation, no /explorer route will be mounted.

Thanks, that explains it.

@bajtos that would require us to leaveRestApplication.component alone and let Application.component take care of mounting components. However, now we'll have to have a list of supported components (Component | RestComponent etc.) in the parameters, IF we don't want RestComponent to extend Component.

const app = new Application(); app.component(RestComponent); app.component(ExplorerComponent);

Good catch! I did not realize we have RestComponent.

I need to think this through, will post a follow-up comment later.

As I commented at the bottom, I think this is a wider problem that we should address outside of this pull request, I opened a new issue for that - see #1967

raymondfeng · 2018-11-01T17:05:53Z

packages/rest/src/rest.application.ts

+    // Assuming components can be synchronously instantiated
+    const instance = this.getSync<Component>(componentKey);
+    mountComponent(this, instance);
+    console.log(instance.staticAssets);


Remove console.log

hacksparrow · 2018-11-01T19:37:23Z

Are you proposing it as a replacement to #1664?

@raymondfeng yes, a different approach.

review 2

raymondfeng · 2018-11-01T20:10:01Z

@raymondfeng yes, a different approach.

Does it assume that API explorer UI is purely based on static assets without any server-side templating/rendering?

raymondfeng · 2018-11-01T20:19:04Z

packages/rest/src/types.ts

+  options?: object;
+}
+
+export interface StaticComponent {


The name StaticComponent does not make sense to me. It's really a component that contributes static assets. I propose to consider #1924 so that we can give more flexibility to component providers. For example, the ExplorerComponent can expose a binding for its static assets so that the RestComponent can discover that and mount it.

Agreed StaticComponent isn't the best name. We can decide on the appropriate name later, for now I chose something to get moving.

hacksparrow · 2018-11-02T05:10:44Z

Does it assume that API explorer UI is purely based on static assets without any server-side templating/rendering?

Yes.

bajtos · 2018-11-02T13:18:14Z

Based on the thread #1957 (comment), it looks like I underestimated the complexity of allowing components to contribute static assets in an indirect way (by exporting an staticAssets array). I am proposing to abandon that approach for now and focus on how to implement self-hosted API explorer using the tools that LB provides right now.

In an older chat with @hacksparrow, we were discussing the following approach:

class ExplorerComponent {
  constructor(
    @inject(CoreBindings.APPLICATION_INSTANCE) app: Application
  ) {
   app.static(/*...*/);
  }
}

I opened a new issue to discuss the best way how to allow extensions to contribute static assets: #1967

hacksparrow · 2018-11-02T13:34:45Z

Although we did not achieve what we wanted to with this approach at this moment, at least we discovered an important missing feature - contributing static assets from components.

I will be opening a new PR for local API Explorer with the approach @bajtos described above.

raymondfeng · 2018-11-02T17:11:59Z

I think that would be just simplified version of #1664 based on the assumption that ExplorerComponent only requires static assets without server-side rendering/templating.

hacksparrow · 2018-11-02T18:34:43Z

I think that would be just simplified version of #1664 based on the assumption that ExplorerComponent only requires static assets without server-side rendering/templating

Yes.

hacksparrow · 2018-11-05T09:20:45Z

Closing this in favor of #1976.

hacksparrow self-assigned this Nov 1, 2018

hacksparrow requested review from bajtos and raymondfeng as code owners November 1, 2018 13:05

hacksparrow changed the title ~~feat: local API Explorer~~ [WIP] feat: local API Explorer Nov 1, 2018

feat: local API Explorer

ff10a9c

Add local API Explorer

hacksparrow force-pushed the api-explorer branch from cdacd6b to ff10a9c Compare November 1, 2018 13:09

bajtos reviewed Nov 1, 2018

View reviewed changes

fix: review 1

f1dfb41

review 1

raymondfeng reviewed Nov 1, 2018

View reviewed changes

dhmlau mentioned this pull request Nov 1, 2018

Support local API Explorer #1960

Closed

3 tasks

fix: review 2

391b6c7

review 2

raymondfeng reviewed Nov 1, 2018

View reviewed changes

bajtos mentioned this pull request Nov 2, 2018

Allow extensions to contribute static assets #1967

Closed

hacksparrow closed this Nov 5, 2018

[WIP] feat: local API Explorer #1957

[WIP] feat: local API Explorer #1957

Uh oh!

Conversation

hacksparrow commented Nov 1, 2018

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

raymondfeng commented Nov 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hacksparrow commented Nov 1, 2018

Uh oh!

raymondfeng commented Nov 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hacksparrow commented Nov 2, 2018

Uh oh!

bajtos commented Nov 2, 2018

Uh oh!

hacksparrow commented Nov 2, 2018

Uh oh!

raymondfeng commented Nov 2, 2018

Uh oh!

hacksparrow commented Nov 2, 2018

Uh oh!

hacksparrow commented Nov 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone