Move RestServer config out of `async start` #621

shimks · 2018-02-15T21:52:35Z

This PR would utilize the new changes made in loopbackio/loopback-next#994 so that async start is not overridden if it can be overridden.

I left a couple of decisions undecided because I wasn't sure whether i should be changing some of these or not. Here are my main concerns:

In the WidgetApplication example under Application.md, server.api(WidgetApi) is used in start. The function call can't be moved into the constructor since the app is only extending from Application and not RestApplication; api() function can't be called because it doesn't exist for the application. Should the example be left as it is?
In Context.md, we configure multiple servers inside the start function. I think this should be left as is, but does anybody think otherwise?

bajtos

This is great!

bajtos · 2018-02-16T08:30:28Z

pages/en/lb4/Defining-and-validating-the-API.md

 ```

-You can then bind the full spec to the application using `server.spec()`. This is done on the server level, because each server instance can expose a different (sub)set of API.
+You can then bind the full spec to the application using `app.api()`. Normally, this is done on the server level because each server instance can expose a different (sub)set of API, but since `RestApplication` uses only one REST server, you can bind the spec at application level.


I am proposing a slightly different phrasing:

You can then bind the full spec to the application using `app.api()`. This works well for applications with a single REST server, because there is only one API definition involved. If you are building an application with multiple REST servers, where each server provides a different API, then you need to call `server.api()` instead.

bajtos · 2018-02-16T08:30:50Z

pages/en/lb4/Routes.md

 The example below defines a `Route` that will be matched for `GET /`. When the `Route` is matched, the `greet` Operation (above) will be called. It accepts an OpenAPI [OperationObject](https://github.com/OAI/OpenAPI-Specification/blob/0e51e2a1b2d668f434e44e5818a0cdad1be090b4/versions/2.0.md#operationObject) which is defined using `spec`.
 The route is then attached to a valid server context running underneath the
-application.
+application. 


Trailing space, please remove.

bajtos · 2018-02-16T08:31:33Z

pages/en/lb4/Routes.md

-  const server = await app.getServer(RestServer);
  const route = new Route('get', '/', spec, greet);
-  server.route(route);
+  app.route(route); // attaches route to RestServer


Please move route registration out of async start() function.

I think we no longer need async start function and can call app.start() directly:

// greet is a basic operation function greet(name: string) { return `hello ${name}`; } const route = new Route('get', '/', spec, greet); app.route(route); app.start();

bajtos · 2018-02-16T08:33:14Z

pages/en/lb4/Sequence.md

 (async function start() {
-  const server = await app.getServer(RestServer);
-  server.sequence(MySequence);
+  app.sequence(MySequence);


Ditto, please move this line out of async start() function and consider removing the start function completely.

shimks · 2018-02-27T16:36:18Z

@bajtos May I get another review on this PR so that it could be landed? Thanks

b-admike

👏

virkt25 · 2018-02-28T17:17:30Z

The function call can't be moved into the constructor since the app is only extending from Application and not RestApplication; api() function can't be called because it doesn't exist for the application. Should the example be left as it is?

I think we should remove this or create a new issue to track the removal. If it's simple just do it in this PR. This way we don't have misleading information in docs that will confuse users and provide a poor experience for anyone following the docs.

shimks · 2018-02-28T17:44:50Z

You're thinking of just removing server.api(WidgetApi) entirely right? That would allow us to move things that are inside start to the constructor, but then the question becomes what is the start function for? What do we advocate users to put inside the start function? I'm not sure what this is myself after we decided on getting rid of multi server support.

virkt25 · 2018-02-28T18:24:58Z

Yes getting rid of server.api() if it doesn't work in the context of that example.

As for what start is, it's not a function user's should need to override ... just call to start their server(s)

bajtos

LGTM, but please consider addressing my first comment to simplify the WidgetApplication example.

bajtos · 2018-03-01T14:42:48Z

pages/en/lb4/Application.md

 import {SamoflangeController, DoohickeyController} from './controllers';
-import {WidgetApi} from './apidef/';

 export class WidgetApplication extends Application {


IMO, you should extend RestApplication here. The only reason why you would not want to, would be if your application was exposing multiple REST servers, which is not the case AFAICT.

This will also allow you to get rid of app.component(RestComponent) below.

The only problem I have with that is the page the docs is about: Application. IMO an example shown in this page should be vanilla Application instead of RestApplication. What do you think?

Fair enough, let's keep using Application then 👍

Could you perhaps extend Tips for application setup to mention RestApplication + when and why to use it? It's ok to leave that out of this pull request though.

bajtos · 2018-03-01T14:46:01Z

pages/en/lb4/Application.md

-    // other components to configure them before launch.
-    const server = await app.getServer(RestServer);
-    server.bind('rest.port').to(8080);
-    server.api(WidgetApi);


This is a breaking change in the context of the original example. The original example assumed api-first approach, where WidgetApi contained Swagger spec describing REST API, with extensions like x-controller-name and x-method used to link Swagger endpoints with controller method implementing them.

Since api-first support is out of scope of our upcoming release, I am ok to rework this example to code-first approach, where the REST API is described on controller classes/methods using decorators like @get and @param.

In other words, this change is fine with me too 👍

b-admike · 2018-03-02T18:07:43Z

pages/en/lb4/Defining-and-validating-the-API.md

-    // inject your spec here!
-    server.api(spec);
-    server.bind("rest.port").to(3001);
-    await super.start();


Don't we still need to call super.start()?

@b-admike I think in this case it's enough to just inherit start() function from server. No need to override it.

b-admike · 2018-03-02T18:09:53Z

pages/en/lb4/Routes.md

+const route = new Route('get', '/', spec, greet);
+app.route(route); // attaches route to RestServer
+
+app.start();


Isn't app.start() returning a promise and thus should be called with await? (see https://github.com/strongloop/loopback-next/blob/master/packages/core/src/application.ts#L128-L136)

No, that's a myth.

await can only be used within an async function. The await keyword helps VM to suspend the execution and wait for the resolution/rejection of the promise or value.

In this case, you cannot use await as the module will be wrapped into a function by Node.js but it's not async.

The code before the change declares an async function and execute it immediately.

It's perfectly fine to call an async function or a function returning a promise without await. The return value is Promise. You have to manually call .then() or .catch() to set up result handler.

Thanks for the clarification Raymond. We don't really need to show .then() or .catch() handlers in this example. Disregard my comment!

b-admike · 2018-03-02T18:15:05Z

pages/en/lb4/Sequence.md

-  server.sequence(MySequence);
-  await app.start();
-})();
+app.start();


jannyHou

Same comment as @b-admike : https://github.com/strongloop/loopback.io/pull/621/files#r171920566

Other than that LGTM 👍 awesome clarification of the current behavior of rest server.

b-admike · 2018-03-02T19:40:59Z

pages/en/lb4/Routes.md

+const route = new Route('get', '/', spec, greet);
+app.route(route); // attaches route to RestServer
+
+app.start();


Thanks for the clarification Raymond. We don't really need to show .then() or .catch() handlers in this example. Disregard my comment!

shimks requested a review from bajtos February 15, 2018 21:52

shimks requested a review from bschrammIBM as a code owner February 15, 2018 21:52

shimks added the needs-priority label Feb 15, 2018

shimks force-pushed the rest-config-out-of-start branch from 9ae1b5d to 30cdc4c Compare February 15, 2018 21:53

bajtos suggested changes Feb 16, 2018

View reviewed changes

b-admike approved these changes Feb 27, 2018

View reviewed changes

shimks added 3 commits February 28, 2018 13:29

move RestServer config out of start

b063904

get rid of async start

af022ee

remove async start in Application.md

d903c1b

shimks force-pushed the rest-config-out-of-start branch from 2f6358b to d903c1b Compare February 28, 2018 18:29

bajtos approved these changes Mar 1, 2018

View reviewed changes

add RestApplication tip

33f00e3

b-admike reviewed Mar 2, 2018

View reviewed changes

jannyHou reviewed Mar 2, 2018

View reviewed changes

b-admike approved these changes Mar 2, 2018

View reviewed changes

shimks changed the title ~~[WIP] move RestServer config out of async start~~ Move RestServer config out of async start Mar 2, 2018

bschrammIBM approved these changes Mar 3, 2018

View reviewed changes

shimks merged commit 80a996a into gh-pages Mar 5, 2018

shimks removed the needs-priority label Mar 5, 2018

shimks deleted the rest-config-out-of-start branch March 5, 2018 15:55

shimks mentioned this pull request Mar 15, 2018

Docs/finish migration from loopback.io loopbackio/loopback-next#1141

Merged

6 tasks

Move RestServer config out of async start #621

Move RestServer config out of async start #621

Uh oh!

Conversation

shimks commented Feb 15, 2018

Uh oh!

bajtos left a comment

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!

shimks commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

b-admike left a comment

Choose a reason for hiding this comment

Uh oh!

virkt25 commented Feb 28, 2018

Uh oh!

shimks commented Feb 28, 2018

Uh oh!

virkt25 commented Feb 28, 2018

Uh oh!

bajtos left a comment

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!

jannyHou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Move RestServer config out of `async start` #621

Move RestServer config out of `async start` #621

shimks commented Feb 27, 2018 •

edited

Loading