Update LoopBack 4 docs with rest refactor changes #475

kjdelisle · 2017-10-05T20:36:41Z

Overview

Refactor the docs to cover off the changes made to the framework architecture

Areas where we previously bound REST-specific functions should now have updated examples that leverage the new method
Testing-Your-Application required a large rewrite because
- It wasn't compatible with the refactor
- It didn't work before, either
  It's now a complete step-by-step set of working JavaScript-based examples that I've validated with my own repository: https://github.com/kjdelisle/hello-loopback/tree/doc-validation

Related Issues

crandmck · 2017-10-05T21:30:21Z

pages/en/lb4/Testing-Your-application.md

+Here is the swagger.json that will hook up your `GreetController` to the
+`/greet` path, and associate the `greet` function with a `GET` request.
+
+{% include code-caption.html content="src/apidef/swagger.json".js" %}


Liquid syntax error (spurious quote). S/be:

{% include code-caption.html content="src/apidef/swagger.json" %}

bajtos · 2017-10-06T07:46:45Z

Testing-Your-Application required a large rewrite

I am going to rewrite that page as part of loopbackio/loopback-next#553 anyways. Thanks for updating the code examples though!

bajtos

I am concerned that the recent changes in server-related API worsened user experience and made the application setup code too complex.

I am proposing to take another look at the current implementation and find a way how to make the setup easier to use, to ensure we keeping the high UX level which we started with.

bajtos · 2017-10-06T07:50:17Z

pages/en/lb4/Context.md


 Let's see this in action:

 ```js
 class MySequence extends DefaultSequence {
  handle(request, response) { // we provide these value for convenience (taken from the Context)
-    const req = this.ctx.getSync('http.request'); // but it is still available in the sequence/request context
+    const req = this.ctx.getSync('rest.http.request');


I'd prefer to not teach new developers about getSync, because it does not work for values requiring async computation. I am proposing to update this example as follows:

class MySequence extends DefaultSequence { // we provide request & response for convenience (taken from the Context) async handle(request, response) { // but they are still available in the sequence/request context const req = await this.ctx.get('rest.http.request'); this.send(`hello ${req.params.name}`); } }

Ah, sorry, I must've glossed over that bit, was just correcting the injection key.

bajtos · 2017-10-06T07:51:21Z

pages/en/lb4/Dependency-Injection.md


-app.bind('authentication.strategy').to(new BasicStrategy(loginUser));
+const server = app.getServer(RestServer); // The REST server has its own context!


getServer returns a promise, doesn't it?

const server = await app.getServer(RestServer);

bajtos · 2017-10-06T07:54:00Z

pages/en/lb4/Dependency-Injection.md


-app.bind('authentication.strategy').to(new BasicStrategy(loginUser));
+const server = app.getServer(RestServer); // The REST server has its own context!
+server.bind('authentication.strategy').to(new BasicStrategy(loginUser));


I feel the necessity to configure things like authentication strategy and sequence handler at server-level (and not at app-level) is rather suboptimal UX. What used to be a single line of sync code before, becomes two lines of async/await code that cannot even happen in application's constructor (because it's async).

Can we come up with a better approach that's easier to use? I am fine to leave that out of scope of this patch if you prefer.

We could easily have the examples use application-level context to do their work, since it's all available at the child context of the server, but then we're potentially encouraging a pattern that might prevent use of separate strategies in multiple server instances.

If we're going to alter the examples to simplify the bindings they work with, we'll need to make a section somewhere that details more advanced usage scenarios so that users properly separate their contextual bindings.

bajtos · 2017-10-06T07:55:42Z

pages/en/lb4/Getting-started.md

 });

 (async function start() {
+  // Grab the REST server instance
+  const server = app.getServer(RestServer);


getServer returns a promise, doesn't it?

I also find it weird that we are configuring the sequence handler every time the server is started, and that there is no sequence handler configured after application's constructor was called. (I think this is sort of an addition to my previous comment.)

bajtos · 2017-10-06T07:56:47Z

pages/en/lb4/Reserved-binding-keys.md

+|`rest.http.response`|`Http.RESPONSE`|`ServerResponse`|The raw `http` response object|
+|`routes.${route.verb}.${route.path}`||`RouteEntry`|Route entry specified in api-spec|
+|`rest.sequence`|`SEQUENCE`|`SequenceHandler`|Class that implements the sequence for your application|
+
 **Sequence Actions binding keys**


I think this title should include "REST" keyword, now that the sequence actions listed below are specific to RestBindings.

bajtos · 2017-10-06T08:04:43Z

pages/en/lb4/Routes.md

+  const route = new Route('get', '/', spec, greet);
+  server.route(route);
+  await app.start();
+})();


This will swallow any errors thrown from start, causing "unhandled promise rejection" warnings. You should add .catch() handler at minimum.

It makes me wonder, can we move methods like route, handler and sequence to a different class, one that does not have any dependencies so that it will always resolve synchronously, and then inject this class as a RestServer dependency?

An mock-up:

// inside RestComponent (or a mixin?) app.rest = new RestServerConfigurator(); app.bind(RestBindings.CONFIGURATOR).toDynamicValue(() => app.rest); // in RestServer constructor class RestServer { constructor( @inject(RestBindings.CONFIGURATOR) configurator: RestServerConfigurator, /*...*/) { // define routes, handler, sequence etc. from the configurator } } // usage in applications app.rest.route('get', '/', spec, greet);

This can be extended to support multiple rest servers by replacing app.rest with app.${serverName} , where the server name is the key used in servers.${serverName} binding and in appConfig.servers object.

Thoughts?

Let's leave this out of scope of this documentation update.

bajtos · 2017-10-06T08:07:14Z

pages/en/lb4/Routes.md

@@ -77,7 +87,7 @@ const spec = {
  }
 };

-app.api(spec);
+server.api(spec);


This feels very wrong to me. The API is tightly coupled with Controllers and controllers are defined at application level. As a user, I'd expect api() method to be available in the same place as controller().

Also, if the API is server-specific, and we have multiple REST servers configured, then each server can be providing a different REST API. Is it a good idea?

This feels very wrong to me. The API is tightly coupled with Controllers and controllers are defined at application level. As a user, I'd expect api() method to be available in the same place as controller().

My understanding was that we might have multiple controllers servicing more than one server, either of the same protocol or otherwise. Combine this with the idea that the OpenAPI spec handling is REST-specific and we end up with server-based API binding instead.

Also, if the API is server-specific, and we have multiple REST servers configured, then each server can be providing a different REST API. Is it a good idea?

I think it's a great idea! Your microservice could have a public-facing API that defines its consumable endpoints, and a private/internal API for infrastructure, exporting logs, administration, or whatever you can think of. Without server-level API binding, these scenarios aren't possible anymore.

Playing "Devil's Advocate" for a moment, if we want to strictly enforce the idea that a LoopBack application is a one-server microservice, then it would make sense to migrate some of this configuration back up to application level, and enforce the idea that you pick one, and only one server component to power the application. This would definitely make sense from a microservice architecture perspective, where you're minimizing the responsibility of each running instance and using other technologies to handle communication between services, log aggregation, etc.

I think trying to simultaneously support both philosophies is easily doable, but could be a documentation and UX nightmare for new users.

Your microservice could have a public-facing API that defines its consumable endpoints, and a private/internal API for infrastructure, exporting logs, administration, or whatever you can think of.

This is a pretty cool idea which makes a lot of sense to me 👍 I agree we should support this use case.

My concern is about ease of use for people starting with the framework, notice how many of our examples increased in size.

Anyways, this is out of scope of this pull request, because we have already updated may other places (package READMEs, example repos) to use the convention described in this pull request. To use a better convention, we will need to update both the docs and those other places.

bajtos · 2017-10-06T08:09:15Z

pages/en/lb4/Sequence.md


 const app = new Application({
-  sequence: MySequence
+  components: [RestComponent],


This is a step backwards in terms of UX, at least IMO. Isn't it possible to configure the sequence via component-specific configuration? Something along the following lines:

const app = new Application({ components: [RestComponent], rest: { sequence: MySequence, } });

I think the current design supports your suggestion, so I'll change it.

jannyHou · 2017-10-06T14:55:58Z

pages/en/lb4/Testing-Your-application-advanced.md

+
+  I know this isn't a part of my PR, which is why I'm only leaving this comment
+  instead of fixing it.
+ -->


when I run npm start on local, there is an error complains

Liquid Exception: Invalid syntax for include tag: content="src/apidef/swagger.json".js" Valid syntax: {% include file.ext param='value' param2='value' %} in pages/en/lb4/Testing-Your-application.md jekyll 3.3.1 | Error: Invalid syntax for include tag: content="src/apidef/swagger.json".js" Valid syntax: {% include file.ext param='value' param2='value' %}

branch gh-pages doesn't fail on it.
This line is not directly modified in PR, maybe somewhere else affects it?

oops I commented in the wrong file that's why I didn't find it, should be
https://github.com/strongloop/loopback.io/pull/475/files#diff-ad241bc342976380fb64dab3b286802bR248
their names are so similar lol

b-admike · 2017-10-06T19:23:50Z

pages/en/lb4/Context.md


 Here is a simple example:

 ```js
 const Application = require('@loopback/core').Application;
 const app = new Application(); // `app` is a "Context"
-class MySequence { ... }
-app.sequence(MySequence);
+class MyController { ... }


Can we revert this since either can be used as an example, but in the Request-level context section we expand on MySequence?

bajtos

LGTM. Let's leave usage improvements that I suggested in my comments out of scope of this pull request.

bajtos · 2017-10-10T12:09:19Z

I am going to land this pull request even if it's not perfect yet, so that I can start working on loopbackio/loopback-next#553

@kjdelisle please open a follow-up pull request to address remaining issues/comments.

I apologize for the inconvenience this may have caused.

Kevin Delisle added 9 commits October 5, 2017 16:31

docs(context): Update to reference rest module

ec5e2bb

docs(di): Update examples with rest usage

6b670fb

docs(getting-started): reference rest package

e84683a

docs(reserved-keys): Reorg keys to match refactor

f53143e

docs(routes): Update to match rest refactor

74a57ff

docs(sequence): Update to match rest refactor

8968890

docs(test-adv): Update to match rest refactor

e0115f3

docs(testing): Rework examples for rest

abab326

docs(TIL): Updated to match rest refactor

a467dc1

kjdelisle self-assigned this Oct 5, 2017

kjdelisle requested review from bschrammIBM and crandmck as code owners October 5, 2017 20:36

kjdelisle added needs-priority review and removed needs-priority labels Oct 5, 2017

kjdelisle changed the title ~~Lb4/use rest package~~ Update LoopBack 4 docs with rest refactor changes Oct 5, 2017

kjdelisle requested review from virkt25, jannyHou, loay, b-admike, dhmlau and shimks October 5, 2017 20:41

crandmck suggested changes Oct 5, 2017

View reviewed changes

bajtos suggested changes Oct 6, 2017

View reviewed changes

Kevin Delisle added 4 commits October 6, 2017 09:44

docs(context): use get instead of getSync

1d3b484

docs(DI): use await for getServer

23b323a

docs(getting-started): use await for getServer

cf39764

docs(binding-keys): clarify sequence title

42bdfcc

jannyHou reviewed Oct 6, 2017

View reviewed changes

docs(testing): Fix spurious quote

28439b3

b-admike reviewed Oct 6, 2017

View reviewed changes

bajtos approved these changes Oct 9, 2017

View reviewed changes

bajtos mentioned this pull request Oct 9, 2017

docs: Testing-Your-Application loopbackio/loopback-next#553

Closed

crandmck approved these changes Oct 9, 2017

View reviewed changes

crandmck removed the request for review from bschrammIBM October 9, 2017 17:46

bajtos merged commit 3cfc79b into gh-pages Oct 10, 2017

bajtos removed the review label Oct 10, 2017

kjdelisle deleted the lb4/use-rest-package branch November 2, 2017 18:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update LoopBack 4 docs with rest refactor changes #475

Update LoopBack 4 docs with rest refactor changes #475

kjdelisle commented Oct 5, 2017 •

edited

Loading

crandmck Oct 5, 2017 •

edited

Loading

bajtos commented Oct 6, 2017

bajtos left a comment

bajtos Oct 6, 2017

kjdelisle Oct 6, 2017

bajtos Oct 6, 2017

bajtos Oct 6, 2017

kjdelisle Oct 6, 2017 •

edited

Loading

bajtos Oct 6, 2017

bajtos Oct 6, 2017

bajtos Oct 6, 2017 •

edited

Loading

bajtos Oct 9, 2017

bajtos Oct 6, 2017

kjdelisle Oct 6, 2017

bajtos Oct 9, 2017

bajtos Oct 6, 2017

kjdelisle Oct 7, 2017

jannyHou Oct 6, 2017

jannyHou Oct 6, 2017

b-admike Oct 6, 2017

bajtos left a comment

bajtos commented Oct 10, 2017


		app.bind('authentication.strategy').to(new BasicStrategy(loginUser));
		const server = app.getServer(RestServer); // The REST server has its own context!

Update LoopBack 4 docs with rest refactor changes #475

Update LoopBack 4 docs with rest refactor changes #475

Conversation

kjdelisle commented Oct 5, 2017 • edited Loading

Overview

Related Issues

crandmck Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

bajtos commented Oct 6, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjdelisle Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos Oct 6, 2017 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

bajtos commented Oct 10, 2017

kjdelisle commented Oct 5, 2017 •

edited

Loading

crandmck Oct 5, 2017 •

edited

Loading

kjdelisle Oct 6, 2017 •

edited

Loading

bajtos Oct 6, 2017 •

edited

Loading