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

commonjs support with import_style option #211

Merged
merged 1 commit into from
Aug 1, 2018

Conversation

zaucy
Copy link
Contributor

@zaucy zaucy commented Jul 27, 2018

Created for #197

Works in tandem with #210

vars["filename"] = filename;

printer->Print(vars, "const proto = {};\n");
if(!package.empty()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a <space> after each if, while, and switch for consistency please?

@stanley-cheung
Copy link
Collaborator

You might have to rebase this as well. Thanks

@@ -290,16 +323,22 @@ class GrpcCodeGenerator : public CodeGenerator {

string file_name;
string mode;
string import_style_str;
ImportStyle import_style;

for (uint i = 0; i < options.size(); ++i) {
Copy link
Contributor Author

@zaucy zaucy Jul 27, 2018

Choose a reason for hiding this comment

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

@stanley-cheung The uint is causing an error for me. Should this not be size_t?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah right. Sorry please change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

}
}

printer->Print(vars, "proto.$package_name$ = require('./$filename$_pb.js');\n\n");
Copy link
Collaborator

@stanley-cheung stanley-cheung Jul 27, 2018

Choose a reason for hiding this comment

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

Does this mean that the proto messages classes have to be generated into one single file named [some name]_pb.js, and can't be generated into one file per message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I'm matching how the protoc compiler outputs it's message with import_style set to commonjs. They output a js file per proto file with the name <proto-file-name>_pb.js.

@stanley-cheung
Copy link
Collaborator

@zaucy Do you mind doing a rebase here?

I am still trying to run the full npm + commonjs example. Just to make sure: you are using the same Echo backend gRPC c++ server + Envoy proxy right? So the only thing I need to swap out is how the frontend HTML calls the JS code right? I suppose one easy way is just set my webserver to point to a directory with index.js + node_modules/ generated from PR #210?

@zaucy
Copy link
Contributor Author

zaucy commented Jul 28, 2018

@stanley-cheung Done!

Yeah all you should have to do is change how the frontend imports the client code. You'll need a frontend that works with commonjs imports. Anything based off webpack would work. Do you want me to write an example in webpack, angular and react? I can do that in another PR early next week.

@stanley-cheung
Copy link
Collaborator

@zaucy Yea such a complete example would greatly benefit the project! Thanks again for your great contributions!

@stanley-cheung
Copy link
Collaborator

stanley-cheung commented Jul 28, 2018

Ha, I was able to (well, use a very roundabout way) finally verify that the CommonJS generated code works end-to-end!

Hack 1: I put all the generated code echo_pb.js and echo_grpc_pb.js into the same directory where package.json is, in the packages/grpc-web folder. And manually hack all the require to be like require('./echo_pb.js');

Hack 2: This line in the grpc-web generated code doesn't work
grpc.web = require('grpc-web');

But because I ran npm run build before, I changed that line to
grpc.web = require('./index.js');

And then I make client.js

const {EchoRequest} = require('./echo_pb.js');                                                                
const {EchoServiceClient} = require('./echo_grpc_pb.js');                                                     
                                                                                                              
const client = new EchoServiceClient('http://localhost:8080', null, null);                                    
                                                                                                              
const request = new EchoRequest();                                                                            
request.setMessage('Hello World!');                                                                           
                                                                                                              
client.echo(request, {}, (err, response) => {                                                                 
  console.log(response.getMessage());                                                                         
});  

And then ./node_modules/.bin/browserify client.js > out.js

And finally an index.html file that has a <script> tag that runs out.js

Is it because I haven't "published" the grpc-web module to npm that I need to hack require('grpc-web'); to require('./index.js');?

Hack 3: npm link (which install the local grpc-web module to my global node_modules and then npm link grpc-web to link the grpc-web module back to my current folder packages/grpc-web

@stanley-cheung
Copy link
Collaborator

stanley-cheung commented Jul 30, 2018

@zaucy I noticed a potential discrepancy on where the generated file is placed for the grpc plugin vs the normal (message) protoc JS plugin:

Let's say my protoc command is like this:

~/grpc-web$ protoc ./net/grpc/gateway/examples/echo/echo.proto --js_out=import_style=commonjs:generated --plugin=protoc-gen-grpc-web=./javascript/net/grpc/web/protoc-gen-grpc-web --grpc-web_out=import_style=commonjs,mode=grpcweb,out=echo_grpc_pb.js:generated

The grpc generated file is placed in ./generated/echo_grpc_pb.js, but the messages (generated by the standard protoc JS plugin) file is placed in ./generated/net/grpc/gateway/examples/echo/echo_pb.js.

Should they be both placed in the ./generated/net/grpc/gateway/examples/echo directory?

But then I do noticed that in echo_grpc_pb.js, including the messages classes work this way:

proto.grpc.gateway.testing = require('./net/grpc/gateway/examples/echo/echo_pb.js'); 

So it does work as is. But it feels weird that the generated files are in a different location.


So perhaps it makes sense to 1. place both files into ./generated/net/grpc/gateway/examples/echo and 2. have echo_grpc_pb.js include the messages via proto.grpc.gateway.testing = require('./echo_pb.js');?

@lalomartins
Copy link

Off the top of my head generators for other languages (I've worked with Python and C# at least, maybe C++) put all output relative to where the command is run, it this case ./generated. Given the way I'd expect to automate generating bindings for multiple languages, my gut feeling is that's the right behavior, and the pb.js file is in the wrong.

And the reasoning is simple: if you want it in the same place as the proto file, that's easy to achieve by running the command with that cwd. In fact, the command line explicitly asks for :generated as the output path, but that's ignored, so I'd call it a bug.

@zaucy
Copy link
Contributor Author

zaucy commented Jul 30, 2018

@stanley-cheung I'll take a look later today and make sure it's consistent.

@stanley-cheung
Copy link
Collaborator

@lalomartins sorry I was a little ambiguous. The _pb.js still honors the ./generated. I edited my post.

One is ./generated/echo_grpc_pb.js. The other is ./generated/net/grpc/gateway/examples/echo/echo_pb.js

@lalomartins
Copy link

Ah, I see what you mean. That's especially important if there are multiple files and they're not all in the same place (like you have a subdirectory — we had a layout like this in a recent project).

@stanley-cheung
Copy link
Collaborator

Nevermind, I can manipulate the protoc command a bit and get the generated files into the same directory.

protoc -I=net/grpc/gateway/examples/echo echo.proto
--plugin=protoc-gen-grpc-web=./javascript/net/grpc/web/protoc-gen-grpc-web
--js_out=import_style=commonjs:generated
--grpc-web_out=import_style=commonjs,mode=grpcweb,out=echo_grpc_pb.js:generated

@stanley-cheung stanley-cheung merged commit 81e7f74 into grpc:master Aug 1, 2018
loyalpartner pushed a commit to loyalpartner/grpc-web that referenced this pull request Sep 4, 2020
* grpcClient.finishSend() is essential for websocket transport

* call hackIntoNormalGrpcRequest() in handleWebSocket()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants