docs(lb4): Creating components - custom servers #523

kjdelisle · 2017-11-09T17:36:46Z

connected to loopbackio/loopback-next#681

raymondfeng · 2017-11-09T18:19:52Z

Good stuff. I wonder why you don't use jsonrpc-2.0? The spec is close to what you illustrate.

virkt25 · 2017-11-09T17:59:21Z

pages/en/lb4/Creating-components.md

+
+## Creating your own servers 
+
+LoopBack 4 has the concept of a Server, which you can use to create your own


which can be used might sound better

@crandmck Do we want to avoid addressing the user? I can switch to passive voice if we want.

Actually, we already have plenty of examples where we're addressing the user:
https://loopback.io/doc/en/lb4/Schemas.html

virkt25 · 2017-11-09T18:01:07Z

pages/en/lb4/Creating-components.md

+
+### A Basic Example
+The concept of servers in LoopBack doesn't actually require listening on ports;
+you can use this pattern to create workers that process data, write logs, or


or do whatever you'd like.

virkt25 · 2017-11-09T18:04:51Z

pages/en/lb4/Creating-components.md

+          $gt: window,
+        },
+      });
+      if (results && results.length > 0) {


results.length > 0 isn't needed. forEach won't throw on an empty array.

Why call it when I can check if it's got elements?
Also, it could end up being defined, but not a collection with a length property.
Mostly just defensive coding, really.

Additional point-of-note: The advanced example is real code that I've tested, but this one was largely a thought experiment. It's definitely not robust (there are plenty of holes in the way it looks for reports to process, for example), and it might not even run at all.

Do we want to make this code snippet something users can copy and paste and run? It'd be cool to get them started, especially since this is our basic example of a server.

If we want this to be copy-pastable, I'd have to expand it to include all of the same boilerplate snippets contained in the advanced example (index.ts, /src/application.ts, etc)

it might not even run at all

Please, let's avoid code that looks like it should run but that does not actually run. If you only want to outline the implementation, then use some sort of a pseudo code.

You can also stick a big fat warning saying the file does not compile, but that would look pretty lame, at least to me. We are writing these code examples to be read by many people, I think the time needed to verify that the code examples actually work is very well worth it!

virkt25 · 2017-11-09T18:15:11Z

pages/en/lb4/Creating-components.md

+and then update the database.
+
+### Using your server
+To use your server, simply bind it to your application's context.


Do we want to call using methods like .controller, .server, etc. as bindings or does it make sense to say To use your server, register it with your application via .server()

virkt25 · 2017-11-09T18:28:55Z

pages/en/lb4/Creating-components.md

+
+### Trying it out
+Let's start our server up and run a few REST requests. Feel free to use
+whatever REST client you'd prefer (this example will use `curl`).


Shouldn't it be in this example

kjdelisle · 2017-11-09T18:31:23Z

Funny, @bajtos said the same thing when I showed him this earlier. This came from an example I made on-the-fly with the team.

Wouldn't making it a jsonrpc-2.0 server complicate the Router portion quite a bit?
You'd need to compile a list of all the methods available on the controller definitions to determine which one is appropriate handler, whereas my contrived example lets you skip that part.

kjdelisle · 2017-11-09T18:32:38Z

@jonathan-casarrubias PTAL

I want to know if this answers some of the questions you have/may have had while working on the gRPC server, as well as what you might want to see more of in this document.

raymondfeng · 2017-11-09T18:56:46Z

Wouldn't making it a jsonrpc-2.0 server complicate the Router portion quite a bit?
You'd need to compile a list of all the methods available on the controller definitions to determine which one is appropriate handler, whereas my contrived example lets you skip that part.

We can use ControllerName.methodName as the method for json-rpc 2.0.

crandmck · 2017-11-09T19:26:41Z

@kjdelisle I just made some copy edits directly to the branch, rather than via comments. Hope that's OK.

bajtos · 2017-11-09T20:05:55Z

I want to know if this answers some of the questions you have/may have had while working on the gRPC server, as well as what you might want to see more of in this document.

Adding @akashjarad to the loop, he volunteered to implement MQTT support - see loopbackio/loopback-next#710 and loopbackio/loopback-next#647 (comment)

kjdelisle · 2017-11-09T20:14:25Z

Rebased, squashed @crandmck 's changes into the first commit

kjdelisle · 2017-11-09T20:21:47Z

We can use ControllerName.methodName as the method for json-rpc 2.0.

What if there are two controllers with the same method name?
Though, I guess you could just document that as a limitation of the server...

Hmm. Maybe I should just write a JsonRpcServer, and document the process? :P

b-admike · 2017-11-09T22:31:29Z

pages/en/lb4/Creating-components.md

+    // Currently, there is a limitation with core node libraries
+    // that prevents the easy promisification of some functions, like
+    // http.Server's listen method.
+    this._server = this.expressServer.listen(


Can we wrap listen() call with try/catch block?

It's meant to be demo code. I don't want to impact its readability with loads of boilerplate safeguarding and error handling.

It's the same reason it's not riddled with debug statements. :P

I am actually quite concerned about this too. People look up to our docs for examples they will be copy-n-pasting into their code and which may eventually get into production.

I agree we should keep this readable, but not at the cost of showing bad practices!

Here in particular we should use p-event to ensure error event reported by the server is correctly handled.

this._server = this.expressServer.listen(...); return await pEvent(this._server, 'listen');

It is also important to wait for listen event before returning from start(), otherwise our app can print "Server is running on port XYZ" before the server has actually started to listen 😱

What's worse, if there are tests calling app.start() and making HTTP requests immediately after start returns, then they can sporadically fail when the timing is wrong and listen takes more ticks of event-loop/promise micro-queue than usually and the request is started before the server started listening.

Including pEvent seems a bit odd. I didn't even know it was a thing (I think you mentioned it in your presentation, but I missed the start!). Not everyone will know what pEvent is/does, and I think it detracts from the example, which is why I stuck the comment up there instead.

It is also important to wait for listen event before returning from start(), otherwise our app can print "Server is running on port XYZ" before the server has actually started to listen 😱

I had a version that handled this using old-school promises:

return new Promise((resolve, reject) => { // using this ugly form to call resolve or reject on the event. });

I wasn't a fan because it detracted from the core logic. I think I'll just use p-event.

b-admike · 2017-11-09T22:31:35Z

pages/en/lb4/Creating-components.md

+    return Promise.resolve();
+  }
+  async stop(): Promise<void> {
+    this._server.close();


see above for response

There is actually no need to wrap this in a try/catch block here. If close() throws, the error will be converted into promise rejection and the promise returned by the async stop() function will be rejected with that.

However, it's probably a good practice to wait until the server is closed, and maybe return an error when it cannot be closed (although I am not sure what the caller is expected to do in such case, as there is little to do when the server cannot be closed).

this._server.close(); return await pEvent(this._server, 'close');

b-admike · 2017-11-09T23:29:24Z

Okay LGTM

bajtos

I reviewed about two thirds of the patch up to "Defining our router and server", PTAL at my comment. I'll take a look at the remaining content later, hopefully still today.

bajtos · 2017-11-10T13:31:52Z

pages/en/lb4/Creating-components.md

+  async start() {
+    // Run the report processor every 2 minutes.
+    let window = Date.now();
+    interval = setInterval(async () => {


I think this created a global interval property, did you mean this.interval?

Also note that errors (promise rejections) returned by this function are ignored and trigger process.on('unhandledRejection'), which is IMO not the right way for handling errors in background processing. You should catch the errors, see e.g. the code snippet in my comment above.

bajtos · 2017-11-10T13:36:16Z

pages/en/lb4/Creating-components.md

+  @inject('database') db: DefaultCrudRepository<Report, int>;
+  interval: NodeJS.Timer;
+  async start() {
+    // Run the report processor every 2 minutes.


Uh oh, I find this example rather complex and difficult to understand. Can we come up with a simpler example? If not, then please at least refactor the code and extract small named functions that will make it easier to comprehend what's going on. For example:

export class ReportProcessingServer implements Server { // ... async start() { // Run the report processor every 2 minutes. this.interval = setInterval( () => this.processReports().catch(err => console.warn('Report processing failed:', err)), 1000 * 120); } async processReports() { const results = await this.getRecentlyAddedRecords(); if (!results || !results.length) return; await Promise.all(results.map(rep => this.processResult(rep))); } // ... }

bajtos · 2017-11-10T13:43:45Z

pages/en/lb4/Creating-components.md

+  interval: NodeJS.Timer;
+  async start() {
+    // Run the report processor every 2 minutes.
+    let window = Date.now();


I see two problems here:

window can be confused with the global variable provided by browsers

In my experience, window usually refers to an interval of time (how many seconds do we want to go into past), not a point (what is the first timestamp we want to check).

I am proposing to use a better name, e.g. timeOfLastRun.

bajtos · 2017-11-10T13:47:41Z

pages/en/lb4/Creating-components.md

+      });
+      if (results && results.length > 0) {
+        results.forEach(async (rep) => {
+          await db.updateById(rep.id, await processReport(rep));


This is horrible in the sense that it does not wait for the update operations to finish and completely ignore any errors that may happen. Please consider using Promise.all and .map instead, see one of my earlier comments above.

bajtos · 2017-11-10T13:50:00Z

pages/en/lb4/Creating-components.md

+          await db.updateById(rep.id, await processReport(rep));
+        }); 
+      }
+      window = Date.now();


bajtos · 2017-11-10T13:53:08Z

pages/en/lb4/Creating-components.md

+```ts
+export class GreetController {
+  basicHello(input: Person) {
+    return `Hello, ${input && input.name || 'World'}!`;


It's a bad practice to mix business logic (what's the name to print) with views/templates. Please split this line into two:

const name = input && input.name || 'World'; return `Hello, ${name}!`;

It's not that bad here, but hobbyHello below is much more difficult to read in the current form. Here is what I am proposing for hobbyHello:

const hobby = input && input.hobby || 'underwater basket weaving'; return `${this.basicHello(input)} I heard you like ${hobby}.`;

(I think this will also make the code easier to debug, I am not sure if V8/DevTools Inspector can step through template parts.)

bajtos

Few more comments regarding the code snippets.

At high level, I am missing any mention of Sequences. I think it's important to explain why we use the Sequence concept in REST, why extension developers should use the Sequence concept for their own transports too, and also show an example how to implement a new Sequence with new sequence actions for a custom transport.

Thoughts?

bajtos · 2017-11-10T15:00:38Z

pages/en/lb4/Creating-components.md

+    this.routing = express.Router();
+    const jsonParser = parser.json();
+    this.routing.post('*', jsonParser, async (request, response) => {
+      console.log(JSON.stringify(request.body));


What if this line throws an error? The top-level async function will return a rejected promise, which will be ignored because express does not report async middleware/route functions.

Please consider cleaning up this code and making it more robust.

class { constructor(...) { this.routing.post('*', jsonParser, (request, response) => { this.handleRequest(request, response) .catch(err => this.handleError(request, response, error)); }); } async handleRequest(request, response) { console.log(JSON.stringify(request.body)); // etc. } handleError(request, response, err) { console.error('Cannot handle %s %s:', request.method, request.path, err); if (response.headersSent) return; response.statusCode = 500; response.end(); }); }

bajtos · 2017-11-10T15:01:32Z

pages/en/lb4/Creating-components.md

+        const controller = await this.server.get(`controllers.${ctrl}`);
+        if (!controller) {
+          response.statusCode = 400;
+          response.send(`No controller was found with name "${ctrl}".`);


I'd personally throw new HttpErrors.BadRequest. However, since the controller was not found, it's probably better to send 404? Also, if this RPC is JSON based, shouldn't we return a JSON formatted error, possibly with 200 status code?

bajtos · 2017-11-10T15:01:49Z

pages/en/lb4/Creating-components.md

+          response.statusCode = 400;
+          response.send(
+            `No method was found on controller "${ctrl}" with name "${method}".`
+          );


bajtos · 2017-11-10T15:05:40Z

pages/en/lb4/Creating-components.md

+      }
+    });
+    // Use our router!
+    this.server.expressServer.use(this.routing);


Why is this custom router a class when all it does is to create an express router and mount it on the server? I find this design confusing. Also why are you creating a new Router instance, when there is only a single route set up?

Here is a possibly simpler solution:

export function mountRpcRoute(server) { server.expressServer.post('*', jsonParser, async (request, response) => { handleRequest(server, request, response) .catch(err => this.handleError(server, request, response, error)); }); }

It was mostly to draw attention to the concept by isolating it as a class.

Additionally, it makes it way simpler to mock pieces of the code if it's a class instead of a function.

bajtos · 2017-11-10T15:07:06Z

pages/en/lb4/Creating-components.md

+    this._server = this.expressServer.listen(
+      (this.config && this.config.port) || 3000
+    );
+    return Promise.resolve();


This line is not needed at all. When an async function does not return anything, the runtime will convert it into a promise resolving to undefined under the hood.

Please remove the other usages of return Promise.resolve() too.

bajtos · 2017-11-10T15:09:20Z

pages/en/lb4/Creating-components.md

+    // Currently, there is a limitation with core node libraries
+    // that prevents the easy promisification of some functions, like
+    // http.Server's listen method.
+    this._server = this.expressServer.listen(


I am actually quite concerned about this too. People look up to our docs for examples they will be copy-n-pasting into their code and which may eventually get into production.

I agree we should keep this readable, but not at the cost of showing bad practices!

Here in particular we should use p-event to ensure error event reported by the server is correctly handled.

this._server = this.expressServer.listen(...); return await pEvent(this._server, 'listen');

bajtos · 2017-11-10T15:11:51Z

pages/en/lb4/Creating-components.md

+    return Promise.resolve();
+  }
+  async stop(): Promise<void> {
+    this._server.close();


There is actually no need to wrap this in a try/catch block here. If close() throws, the error will be converted into promise rejection and the promise returned by the async stop() function will be rejected with that.

However, it's probably a good practice to wait until the server is closed, and maybe return an error when it cannot be closed (although I am not sure what the caller is expected to do in such case, as there is little to do when the server cannot be closed).

this._server.close(); return await pEvent(this._server, 'close');

bajtos · 2017-11-10T15:12:57Z

pages/en/lb4/Creating-components.md

+    // Currently, there is a limitation with core node libraries
+    // that prevents the easy promisification of some functions, like
+    // http.Server's listen method.
+    this._server = this.expressServer.listen(


It is also important to wait for listen event before returning from start(), otherwise our app can print "Server is running on port XYZ" before the server has actually started to listen 😱

What's worse, if there are tests calling app.start() and making HTTP requests immediately after start returns, then they can sporadically fail when the timing is wrong and listen takes more ticks of event-loop/promise micro-queue than usually and the request is started before the server started listening.

bajtos · 2017-11-10T15:15:49Z

pages/en/lb4/Creating-components.md

+  await app.stop();
+  process.exit(0);
+}
+// Event handlers for when we are killed; these aren't *required*.


Well, if we want to keep our examples in this page short and readable, then I think this section should be removed, since it's not really needed? If you want to show how to use stop, then perhaps use an end-to-end test as the example and call start/stop from before/after hooks.

raymondfeng · 2017-11-10T15:43:03Z

It might be better off if we create a github repo to host the sample code and README. The doc site can just make a reference to the README.

kjdelisle · 2017-11-10T22:21:18Z

At high level, I am missing any mention of Sequences. I think it's important to explain why we use the Sequence concept in REST, why extension developers should use the Sequence concept for their own transports too, and also show an example how to implement a new Sequence with new sequence actions for a custom transport.

My thoughts are that Sequence is not demonstrably a best practice, and that it would only serve to complicate a developer's implementation if they originally intended to use middleware chaining or some other pattern.

In our case, a router is a natural consequence of our framework's opinion regarding the use of controllers but the same can't be said for something like sequence.

kjdelisle · 2017-11-10T22:21:53Z

@raymondfeng I'm starting to think so as well. I actually have the code handy, so I might just create a new repository for it and edit the PR to point there.

kjdelisle · 2017-11-13T21:44:32Z

@bajtos I've moved the code to a different repo, PTAL

Comments regarding code no longer relevant, migrated to new repository.

bschrammIBM · 2017-11-13T23:30:19Z

Do you still me to review this? There seems to be a long history.

kjdelisle · 2017-11-14T20:56:28Z

@bschrammIBM It's already landed, but if there are concerns or comments you'd like addressed, you can still perform a review after the fact and I can try to address them in a follow-up PR.

jonathan-casarrubias · 2017-11-15T00:14:37Z

@kjdelisle sorry for the late response...

Anyway I think this is a good start but my first impression is that still is too basic for more complex extensions, I'm not sure how to improve the docs without creating a tutorial instead, but the Server example is not quite the same as the REST and gRPC components are actually built...

Somehow the Server examples demonstrate how to extend an Application but for me it tastes more like an application developer level, rather than an extension developer level example

kjdelisle self-assigned this Nov 9, 2017

kjdelisle requested review from bschrammIBM and crandmck as code owners November 9, 2017 17:36

kjdelisle added review needs-priority team-apex and removed review needs-priority labels Nov 9, 2017

virkt25 reviewed Nov 9, 2017

View reviewed changes

crandmck approved these changes Nov 9, 2017

View reviewed changes

kjdelisle force-pushed the lb4/custom-server-components branch from f9320d4 to 965c11b Compare November 9, 2017 20:14

b-admike reviewed Nov 9, 2017

View reviewed changes

kjdelisle force-pushed the lb4/custom-server-components branch from 965c11b to d85132c Compare November 9, 2017 22:45

bajtos suggested changes Nov 10, 2017

View reviewed changes

bajtos previously requested changes Nov 10, 2017

View reviewed changes

kjdelisle mentioned this pull request Nov 13, 2017

feat(main): RPC Router and Server implementation strongloop-archive/loopback4-example-rpc-server#1

Merged

kjdelisle force-pushed the lb4/custom-server-components branch from d85132c to cc0fc02 Compare November 13, 2017 20:43

docs(lb4): Creating components - custom servers

111f727

kjdelisle force-pushed the lb4/custom-server-components branch from cc0fc02 to 111f727 Compare November 13, 2017 21:19

jannyHou approved these changes Nov 13, 2017

View reviewed changes

virkt25 approved these changes Nov 13, 2017

View reviewed changes

kjdelisle merged commit 8842d4e into gh-pages Nov 13, 2017

kjdelisle deleted the lb4/custom-server-components branch November 13, 2017 23:15

kjdelisle removed the review label Nov 13, 2017


		## Creating your own servers

		LoopBack 4 has the concept of a Server, which you can use to create your own

docs(lb4): Creating components - custom servers #523

docs(lb4): Creating components - custom servers #523

Conversation

kjdelisle commented Nov 9, 2017 • edited Loading

raymondfeng commented Nov 9, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjdelisle commented Nov 9, 2017

kjdelisle commented Nov 9, 2017

raymondfeng commented Nov 9, 2017

crandmck commented Nov 9, 2017

bajtos commented Nov 9, 2017

kjdelisle commented Nov 9, 2017

kjdelisle commented Nov 9, 2017

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

b-admike commented Nov 9, 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

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

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

Choose a reason for hiding this comment

raymondfeng commented Nov 10, 2017

kjdelisle commented Nov 10, 2017

kjdelisle commented Nov 10, 2017

kjdelisle commented Nov 13, 2017

bschrammIBM commented Nov 13, 2017

kjdelisle commented Nov 14, 2017

jonathan-casarrubias commented Nov 15, 2017 • edited Loading

kjdelisle commented Nov 9, 2017 •

edited

Loading

jonathan-casarrubias commented Nov 15, 2017 •

edited

Loading