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

documentation: added instructions for writing gRPC clients in Javascript #125

Merged

Conversation

mably
Copy link
Contributor

@mably mably commented Feb 3, 2017

No description provided.

@mably mably force-pushed the topic/grpc-javascript-tutorial branch 2 times, most recently from 2446c9f to 2f4d5e4 Compare February 4, 2017 00:26
@mably mably force-pushed the topic/grpc-javascript-tutorial branch from 2f4d5e4 to b68951c Compare February 4, 2017 00:27
@Roasbeef Roasbeef force-pushed the master branch 2 times, most recently from d86107c to a34df2f Compare February 8, 2017 04:54
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I've tested out your tutorial locally and everything seems to be working properly.

As a minor nit, the subsystem prefix on the PR should instead be docs/grpc.

Also, I think it'd be helpful for new comers if we linked to the official gRPC tutorial for Javascript in the documentation: http://www.grpc.io/docs/tutorials/basic/node.html

I had to comment the following line in the `rpc.proto` file otherwise `grpc` would crash:

```
//import "google/api/annotations.proto";
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this looks related to: protobufjs/protobuf.js#433

We can either investigate a direct fix, or just re-word the statement above to be declarative:

In order to use the protos from js, you'll need to comment out the following line.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

Merging this as is. I'll add the additional links myself. Thanks!

LGTM 🏂

@Roasbeef Roasbeef merged commit b2aa45d into lightningnetwork:master Mar 1, 2017
@mably
Copy link
Contributor Author

mably commented Mar 1, 2017

Thanx @Roasbeef for merging my PR.

Simple question, sorry for my ignorance, but what is the PR subsystem prefix you are talking about exactly?

@mably mably deleted the topic/grpc-javascript-tutorial branch March 1, 2017 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants