Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature request: grpc-web client in commonjs + npm package #197

Closed
glerchundi opened this issue Jun 16, 2018 · 10 comments
Closed

feature request: grpc-web client in commonjs + npm package #197

glerchundi opened this issue Jun 16, 2018 · 10 comments

Comments

@glerchundi
Copy link
Collaborator

We found lots of issues when trying to integrate the current gRPC Web Client implementation into our stack because we (and probably most of the non-Googlers) don't use Google Closure for nothing.

I would like to see the client library released in npm and commonjs compliant which probably would solve all the issues we've had. Javascript Protocol Buffers generator already supports commonjs so it seems like a good idea to have a re-implementation of the client in this way.

WDYT? Is this on the roadmap?

@jonahbron
Copy link
Collaborator

jonahbron commented Jun 18, 2018

My project is using Improbable's GRPC client and proxy (CommonJS), and it's working really well. Might pay off to use it until this official implementation matures a bit more for external use.

https://github.com/improbable-eng/grpc-web

@glerchundi
Copy link
Collaborator Author

@jonahbron thanks for the pointers but we prefer to stick with an official development of grpc-web although it's in closure instead of relying on a third-party implementation.

@wenbozhu
Copy link
Member

The closure dependency is internal to grpc-web, unless you want to use closure compiler (which I heard TypeScript uses too internally).

Agreed that we should add npm support esp. if protobuf generator already supports this. We will get back to you on timelines.

@n0mer
Copy link
Contributor

n0mer commented Jul 11, 2018

@wenbozhu good news, thanks - looking forward as well!

@stanley-cheung
Copy link
Collaborator

May I ask the community what the expectation for this feature request is?

  1. Right now, the JS code in the core / generic gRPC-Web library (i.e. stuff in this directory https://github.com/grpc/grpc-web/tree/master/javascript/net/grpc/web) uses the goog.module, good.require include style. I can certainly make them commonjs style (i.e. require, module.exports). Is that what's expected here?

  2. How does the protoc plugin generated code fit in? The current expectation is that, you have a .proto definition. 2a. You run our gRPC-Web protoc plugin. That generates the gRPC-Web API .JS client stub. 2b. With this, and code above, you run the closuer-compiler.jar to generate one single, compiled, JS file which has no other runtime dependencies.

With the output of 2b, you are free to embed that into whatever frontend framework you will be using. Step 2a and 2b should be mostly 1-time build step. How does the gRPC-Web core library code being a npm module help in this case?

I may have missed something obvious so please let us know how you guys think this should work and I will be happy to help.

@zaucy
Copy link
Contributor

zaucy commented Jul 12, 2018

Having the option for the import style to be commonjs and having all common-functionality in an npm module is what my expectation would be. Here's a simple example:

import {StatusCode} from 'grpc-web';

// ...

if(code == StatusCode.ABORTED) {
  // ...
}

Instead of accessing like this

// ...

if(code == grpc.web.StatusCode.ABORTED) {
  // ...
}

This way I wouldn't need to run the closure compiler. I could just have my generated js files locally in my project or in it's own private npm module itself. This would integrate really nicely with webpack.

import {EchoServiceClient} from './path/to/generated/client';
import {EchoRequest} from './path/to/generated/message';
// or
import {EchoServiceClient} from '@my-private/npm-package';

let echoService = new EchoServiceClient(...);
let echoRequest = new EchoRequest();
echoRequest.setMessage("hello world");

echoService.echo(echoRequest, ...);

We are currently using the https://github.com/improbable-eng/grpc-web ts-protoc-gen plugin for generating our js. We have a published private npm module that contain the generated js from the plugin as well as grpc-web-client as a peer dependency of each. grpc-web-client would be like the grpc-web's npm module in this case.

If grpc-web was available as an npm module as I described above it would make it much easier for us to integrate it with our current projects.

@stanley-cheung
Copy link
Collaborator

I see. If I understand correctly, you would like to use both the generated code (client stub), and the grpc-web core library "as-is", without the compilation step (side note: the compilation step does have added benefits like dead code removal, which leads to smaller code size). And you are OK with having the grpc-web core library pulled in as runtime dependency. Is that correct?

If that's correct, then I would prefer to keep the current goog.module and closure compilation step as the primary distribution for now, because for a lot of the internal uses, having small code size and no runtime dependency is very important.

I can figure out if we can provide a separate npm module that can be used "as-is", now that I understand the ask better. Thanks!

@zaucy
Copy link
Contributor

zaucy commented Jul 12, 2018

I see. If I understand correctly, you would like to use both the generated code (client stub), and the grpc-web core library "as-is", without the compilation step [...] And you are OK with having the grpc-web core library pulled in as runtime dependency. Is that correct?

Precisely and yes!

(side note: the compilation step does have added benefits like dead code removal, which leads to smaller code size).

Other build solutions can achieve similar dead code removal with the commonjs imports/exports. Webpack tree shaking, rollup, uglifyjs etc.

If that's correct, then I would prefer to keep the current goog.module and closure compilation step as the primary distribution for now, because for a lot of the internal uses, having small code size and no runtime dependency is very important.

Completely understand.

Would it make sense if there was an import_style (similar to the --js_out=import_style=commonjs) option and it was set to commonjs that it would use the runtime dependency?

@stanley-cheung
Copy link
Collaborator

Would it make sense if there was an import_style (similar to the --js_out=import_style=commonjs) option and it was set to commonjs that it would use the runtime dependency?

Yes I can add this option to the codegen plugin. I still need to figure out how to make two different styles of require's co-exist in the core library though.

@zaucy
Copy link
Contributor

zaucy commented Jul 27, 2018

I've created 2 pull requests that include an npm module for grpc-web runtime as well as protoc plugin changes to support commonjs (#210 and #211 respectively.) They were converted from my local build that have the prefix @grpc-gen/. Additionally I have a demo that uses the runtime here: https://github.com/zaucy/grpc-chat-demo (with the @grpc-gen/ prefix)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants