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

Pass proto root and proto file to grpc.load #24

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

zachgrayio
Copy link

I made a small change to pass proto root and proto file to grpc.load(), allowing for correct inclusion of additional protobuf files.

The interface becomes the following:

export declare function clientFactory<T>(protoFile: string, protoRoot: string, packageName: string): ClientFactoryConstructor<T>;

This allows us to do something like this in our service definitions, assuming that the imported proto files are available in the path provided when calling the above function:

import "google/api/annotations.proto";
...
  rpc Echo(StringMessage) returns (StringMessage) {
        option (google.api.http) = {
            post: "/user/echo"
            body: "*"
        };
  }

Unfortunately this isn't the cleanest pull request - I haven't had a chance to get the tests working because I can't seem to get them running on my machine. I also had to use Node v7.10.1 and NPM v4.2.0 and make some tweaks to package.json to get the thing building (there was a yarn.lock but not package-json.lock in master).

Getting this fork to a mergeable state shouldn't require much more effort, but I don't want to spend much time on it if there's not any interest in merging it.

@zachgrayio
Copy link
Author

zachgrayio commented Mar 23, 2018

it seems this requires a few more changes to function properly but we're actively working on it, will update PR with details soon.

update: this is working great for our use case, but we still need to do some work to get the tests updated and passing.

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