Setup starter, ci and implemented POC / Unary Call. #8

jonathan-casarrubias · 2017-11-02T19:36:41Z

Implemented a vanilla version of the loopback extension
starter, made some tweaks and added travis & appveyor config files.

Added lint fix to pretest
Implemented grpc Decorator
Implemented grpc server provider
Implemented grpc server
Implemented grpc sequence
Implemented grpc bindings
Implemented grpc configs
Implemented unit tests

raymondfeng · 2017-11-02T20:47:08Z

src/decorators/rpc.decorator.ts

@@ -0,0 +1,35 @@
+// Copyright IBM Corp. 2017. All Rights Reserved.


Can we name it grpc.decorator.ts?

raymondfeng · 2017-11-02T20:47:25Z

src/decorators/rpc.decorator.ts

+ *   }
+ * } 
+ */
+export function rpc() {


raymondfeng · 2017-11-02T20:48:07Z

test/unit/decorators/rpc.decorator.unit.ts

@@ -0,0 +1,56 @@
+// Copyright IBM Corp. 2017. All Rights Reserved.


raymondfeng · 2017-11-02T20:49:17Z

@jonathan-casarrubias Great start! I added a few minor comments.

jonathan-casarrubias · 2017-11-02T21:01:54Z

@raymondfeng thanks for reviewing, also let me elaborate why I'm using rpc.

From gRPC Docs

What we are actually implementing with the @rpc decorator are RPC Methods (gRPC stands for the entire RPC System), IMO following semantics would help understand easier to people the reading of the GRPC documentation vs LB4 implementation.

For instance, the entire module is named grpc.component which attempts to achieve the entire gRPC functionality, rpc stands just for RPC methods.

Thoughts?

raymondfeng · 2017-11-02T21:21:56Z

I see. With more extensions contributing decorators, there are potentially name conflicts. Maybe we should use a namespace such as @grpc.rpc but that might be verbose. I guess @grpc at method level is still fine.

jonathan-casarrubias · 2017-11-02T21:29:36Z

@raymondfeng I see your point too and for that I have a couple of notes

1.- I see a little odd -but possible- people using 2 RPC systems within LB at the same time. But if that is the case, then:
2.- That usually is fixed at language level:

import {rpc as grpc} from '@loopback/grpc';
import {rpc as orpc} from '@other/orpc';

That way the decorator maintains the original semantic and package conflicts would be fixed at imports.

Maybe I'm being to squared in terms of semantics, but I still think that can be easily fixed using what I describe above, at the end.... They -the developers- would also be able to import another grpc package that would conflict with the decorator and the fix would be the same I describe above.

for instance, importing the grpc module import * as grpc from 'grpc' while also trying to import the decorator import {grpc} from '@loopback/grpc'.

Anyway if you still believe @grpc is better for any reason, then we can just change that and won't hurt that much :P as if we throw the rest of the code away.... lol

Thoughts?

raymondfeng · 2017-11-02T22:03:37Z

I'm leaning toward @grpc as rpc is too generic. If a developer prefers @rpc, he/she can do the following (as you proposed :-):

import {grpc as rpc} from '@loopback/grpc`;

jonathan-casarrubias · 2017-11-02T22:48:01Z

@raymondfeng ok cool, lets do that then... I'll change to grpc all around.

Cheers!
Jon

jonathan-casarrubias · 2017-11-02T23:07:20Z

@raymondfeng I just did those changes and squached the commit... Now everything related to the decorator is referenced as grpc instead of rpc.

cheers 😄
Jon

bajtos · 2017-11-03T08:48:30Z

@jonathan-casarrubias would you mind moving the project-setup-related changes to a standalone PR, so that we can land it quickly and get those changes out of our way? Quite often, I review pull requests in whole, and it costs me a lot of time and energy to always skip changes that are not important for the feature being implemented.

bajtos · 2017-11-03T08:51:56Z

package-lock.json

IMO, package-lock is pretty harmful when developing modules. When you clone this repo and install dependencies, you get the locked down version of the deps, which is different from the versions that will be installed by our users, when they install this module from npm. What's even worse, our CI will do the same, therefore we will be testing our code against a different set of dependencies that our actual users will use.

The solution: add .npmrc file to disable package-lock functionality and remove package-lock.json file.

bajtos

Great start!

bajtos · 2017-11-03T08:58:59Z

src/decorators/README.md

+    */
+    @grpc()
+    public sayHello(call, callback) {
+      callback(null, {message: 'Hello ' + call.request.name});


I have two comments about this example.

How is Greeter#sayHello linked to the correct service/procedure from the proto file?

It's the task of the transport layer to convert the transport-specific request data into typed and validated arguments of a controller method. The controller methods should not need to deal with low-level transport-specific API like call.request.name, this should be handled by the framework.

class MyController implements Greeter { sayHello({name}: HelloRequest): HelloResponse { return {message: `hello ${name}`}; } }

Here are some of the benefits of this approach:

The user can focus on writing the business logic in typescript/javascript, possibly in a transport-independent way.

Because both request and response are typed (based on the information we already have in the .proto file), the TypeScript compiler can catch many errors at compile time.

bajtos · 2017-11-03T09:02:41Z

test/unit/decorators/grpc.decorator.unit.ts

+  it('defines reflection metadata for rpc method', () => {
+    class Greeter implements GreeterInterface {
+      @grpc()
+      SayHello(request: HelloRequest): HelloReply {


Nice! This is what I was looking for in my previous comment. Looks like the implementation is good but the README needs an update?

yes the readme is the one outdated

Done I updated the docs and addressed the rest of the comments, I'm landing this PR now :)

bajtos · 2017-11-03T09:07:23Z

test/unit/decorators/grpc.decorator.unit.ts

+    const flags: {[key: string]: boolean} = {};
+    const app: Application = givenApplication();
+    app.controller(Greeter);
+    const controller = app.find('controllers.Greeter').pop();


Since this code is living in the file called grpc.decorator.unit.ts, it should contain a unit test that's testing the decorator in isolation, stubbing any dependencies as needed. You should not be using the full Application object! Unfortunately, we are behind on docs for extension developers (see loopbackio/loopback-next#579), but some relevant information can be found in these places:

http://loopback.io/doc/en/lb4/Testing-your-application.html#types-of-tests

http://loopback.io/doc/en/lb4/Testing-your-application.html#unit-testing

In this particular case, just scan Greeter.prototype directly.

let controllerMethods: string[] = []; const flags: {[key: string]: boolean} = {}; const proto = Greeter.prototype; // ...

bajtos · 2017-11-03T09:09:55Z

test/unit/decorators/grpc.decorator.unit.ts

+    }
+    expect(controllerMethods).to.eql(['SayHello', 'Helper']);
+    expect(flags.SayHello).to.be.true();
+    expect(flags.Helper).to.be.undefined();


When something goes wrong here, e.g. there is a typo in SayHello, then these two checks won't provide enough information to troubleshoot the problem.

A better approach is to use deepEqual and test the whole object.

expect(flags).to.deepEqual({ SayHello: true, // Helper: ignored });

bajtos · 2017-11-03T09:11:45Z

test/unit/grpc.component.unit.ts

+describe('GrpcComponent', () => {
+  // GRPC Component Configurations
+  it('defines grpc component configurations', async () => {
+    const app: Application = givenApplication();


Ditto - unit tests should not depend on other objects like Application. I think you should move this file to test/acceptance/grpc.component.acceptance.ts.

@virkt25 you have the most practical experience with testing a component. What's your recommendation - how to test the component itself?

Implemented a vanilla version of the loopback extension starter, made some tweaks and added travis & appveyor config files. Added lint fix to pretest Implemented grpc Decorator Implemented grpc server provider Implemented grpc server Implemented grpc sequence Implemented grpc bindings Implemented grpc configs Implemented unit tests - Fix: #7 - Fix: #4 - Fix: #3

jonathan-casarrubias self-assigned this Nov 2, 2017

jonathan-casarrubias requested review from raymondfeng, bajtos and VMois November 2, 2017 19:36

jonathan-casarrubias force-pushed the jcasarrubias/playground branch 4 times, most recently from 8dd1769 to 9da53f8 Compare November 2, 2017 19:59

raymondfeng reviewed Nov 2, 2017

View reviewed changes

src/decorators/rpc.decorator.ts Outdated

* }

* }

*/

export function rpc() {

Copy link

Contributor

raymondfeng Nov 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpc?

raymondfeng reviewed Nov 2, 2017

View reviewed changes

test/unit/decorators/rpc.decorator.unit.ts Outdated

@@ -0,0 +1,56 @@

// Copyright IBM Corp. 2017. All Rights Reserved.

Copy link

Contributor

raymondfeng Nov 2, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

grpc?

jonathan-casarrubias force-pushed the jcasarrubias/playground branch from 9da53f8 to 86ddab5 Compare November 2, 2017 23:05

jonathan-casarrubias force-pushed the jcasarrubias/playground branch from 86ddab5 to 798b023 Compare November 2, 2017 23:11

bajtos reviewed Nov 3, 2017

View reviewed changes

bajtos mentioned this pull request Nov 3, 2017

Setup CI #3

Closed

jonathan-casarrubias force-pushed the jcasarrubias/playground branch from 798b023 to fc64d21 Compare November 3, 2017 17:14

jonathan-casarrubias force-pushed the jcasarrubias/playground branch from fc64d21 to 10bd14c Compare November 3, 2017 17:18

jonathan-casarrubias merged commit 6cb01f4 into master Nov 3, 2017

jonathan-casarrubias mentioned this pull request Nov 3, 2017

Required Decorators Analysis #6

Closed

VMois mentioned this pull request Nov 3, 2017

npm install failed after starter setup merge #10

Closed

bajtos mentioned this pull request Nov 8, 2017

Calling for Contributors on LB Extensions loopbackio/loopback-next#647

Closed

dhmlau mentioned this pull request Nov 8, 2017

Extension: MQTT loopbackio/loopback-next#710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Setup starter, ci and implemented POC / Unary Call. #8

Setup starter, ci and implemented POC / Unary Call. #8

jonathan-casarrubias commented Nov 2, 2017 •

edited by bajtos

Loading

raymondfeng Nov 2, 2017

raymondfeng Nov 2, 2017

raymondfeng Nov 2, 2017

raymondfeng commented Nov 2, 2017

jonathan-casarrubias commented Nov 2, 2017 •

edited

Loading

raymondfeng commented Nov 2, 2017

jonathan-casarrubias commented Nov 2, 2017 •

edited

Loading

raymondfeng commented Nov 2, 2017

jonathan-casarrubias commented Nov 2, 2017

jonathan-casarrubias commented Nov 2, 2017

bajtos commented Nov 3, 2017

bajtos commented Nov 3, 2017

bajtos left a comment

bajtos Nov 3, 2017

bajtos Nov 3, 2017

jonathan-casarrubias Nov 3, 2017

jonathan-casarrubias Nov 3, 2017

bajtos Nov 3, 2017

bajtos Nov 3, 2017

bajtos Nov 3, 2017

		@@ -0,0 +1,35 @@
		// Copyright IBM Corp. 2017. All Rights Reserved.

		@@ -0,0 +1,56 @@
		// Copyright IBM Corp. 2017. All Rights Reserved.

Setup starter, ci and implemented POC / Unary Call. #8

Setup starter, ci and implemented POC / Unary Call. #8

Conversation

jonathan-casarrubias commented Nov 2, 2017 • edited by bajtos Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raymondfeng commented Nov 2, 2017

jonathan-casarrubias commented Nov 2, 2017 • edited Loading

raymondfeng commented Nov 2, 2017

jonathan-casarrubias commented Nov 2, 2017 • edited Loading

raymondfeng commented Nov 2, 2017

jonathan-casarrubias commented Nov 2, 2017

jonathan-casarrubias commented Nov 2, 2017

bajtos commented Nov 3, 2017

bajtos commented Nov 3, 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

jonathan-casarrubias commented Nov 2, 2017 •

edited by bajtos

Loading

jonathan-casarrubias commented Nov 2, 2017 •

edited

Loading

jonathan-casarrubias commented Nov 2, 2017 •

edited

Loading