loopbackio / loopback4-extension-grpc

gRPC Extension for LoopBack 4
Other
18 stars 8 forks source link

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

Closed jonathan-casarrubias closed 7 years ago

jonathan-casarrubias commented 7 years ago

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 commented 7 years ago

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

jonathan-casarrubias commented 7 years ago

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

From gRPC Docs image

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

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

Cheers! Jon

jonathan-casarrubias commented 7 years ago

@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 commented 7 years ago

@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 commented 7 years ago

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.