include multiple import path?

[c0b]

the protoc compilation support --proto_path= to be specified multiple times, but this project seems not? because in a real large project, there are multiple proto definition files in a complex hierarchy ...

$ protoc --help
Usage: protoc [OPTION] PROTO_FILES
Parse PROTO_FILES and generate output based on the options given:
  -IPATH, --proto_path=PATH   Specify the directory in which to search for
                              imports.  May be specified multiple times;
                              directories will be searched in order.  If not
                              given, the current working directory is used.

  -oFILE,                     Writes a FileDescriptorSet (a protocol buffer,
    --descriptor_set_out=FILE defined in descriptor.proto) containing all of
                              the input files to FILE.

I read the code seems requiring the include to be one string only?
https://github.com/konsumer/grpc-dynamic-gateway/blob/master/index.js#L37

While, I've tried to touch the code a bit if I can make it working, but the grpc.load({ file: p, root: include }) call seems undocumented? the Documentation will definitely need to be improved, while, do you know if it support multiple include proto path?
https://grpc.io/grpc/node/grpc.html#.load__anchor

Another possibility is I see protoc can compile the complex hierarchy *.proto into a single protoset binary file, and it's well supported by tools like grpcurl Are you aware if nodejs-grpc has similar protoset-files support? And add into this tool if possible

https://github.com/fullstorydev/grpcurl#protoset-files

[konsumer]

Yeh, the underlying library that is doing the parsing here (grpc) only supports a single include-path.

The idea of this tool is to quickly get a node-based server running from a single dir, but I agree it's a bit annoying to have to structure everything in a single dir. If grpc lib supported multiple paths, I could add support to this lib. I'm a bit short on time (PRs accepted!) , but I'd like to eventually do my own grpc parsing with support for multiple paths.

[c0b]

this grpc site is supposed to have best documentation about grpc node API references, without Documentation, how did you know that grpc.load({ file: p, root: include }) working? by reading all its source code or what? and is it possible there might be another parameter let grpc.load support multiple import paths?
https://grpc.io/grpc/node/grpc.html#.load__anchor

and if all proto files compiled into a single protoset binary file, is there any nodejs code can parse the protoset binary format?
https://github.com/fullstorydev/grpcurl#protoset-files

[konsumer]

I'm not sure I understand your question. I didn't make the library/tool you are linking to.

[c0b]

I am aware the grpc-node and the grpcurl is not what you made, but since the grpc.load method is not well documented, I was asking how did you know it accept a { file: p, root: include } as parameter? and you may happen to know more about it?

from its API reference https://grpc.io/grpc/node/grpc.html#.load__anchor the documentation is only one line:

<static> load
Load a gRPC object from a .proto file.

I agree only the multiple import path part is an issue filed here, other parts of my questions are better to the mailing list, you can see from this thread: I got to know your project since someone's introduction,

https://groups.google.com/d/msg/grpc-io/dAJz2sPwir8/TlgLny-PBgAJ

the grpc protoset binary format to nodejs is an open question to the grpc node community actually, I asked you just because you may happen to know more about grpc-node internals...

[c0b]

so grpc/grpc-node#556 (comment) is saying grpc.load is deprecated,

should use @grpc/proto-loader instead, this one looks like support includeDirs | An array of strings ? for A list of search paths for imported .proto files.

https://www.npmjs.com/package/@grpc/proto-loader#usage

[nicolasnoble]

I'd like to mention that the grpc.load deprecation should've been producing a runtime warning when using it the first time. I'm surprised this wasn't noticed, and I'd love to hear why it wasn't.

[konsumer]

@nicolasnoble PRs are accepted, but this is an open-source project I work on for free, when I have time. If you noticed it, why didn't you make a PR?

[nicolasnoble]

You don't understand what I am saying.

I'm one of the authors of grpc, and I was directed here by @c0b's cross mention. We added the deprecation of grpc.load in a way that it displays a message at runtime, so as the author of the package, I'm wondering why you haven't noticed this deprecation message, and what we could've done better to properly convey this deprecation to developers who are using our package.

[konsumer]

@nicolasnoble Maybe we were misunderstanding each other, a little. You said:

I'm surprised this wasn't noticed, and I'd love to hear why it wasn't.

I was attempting to answer your "I'd love to hear why it wasn't." I am saying that I don't have as much time for testing this project and following up with flaws in it's dependencies as I would like, but am happy to accept PRs. I originally worked pretty hard to support multiple import paths, with your project, as well as protobufjs, which it depended on. At the time, I realized that it would take more time than I had to fix it all. There are big improvements/changes in your upstream grpc, and I'd love to get them in here and other projects I made that use grpc. It looks like I also have an issue in the queue about the deprecation warning (#13 ), but I don't have time, right now, to fix it, and the multiple-include path issue we are both commenting on. I am also saying that you, @c0b, or @WoLfulus (the reporter on the deprecation issue) are welcome to make PRs that address it, and I will try to get them in there and published as quickly as possible, but other than that I don't really have time to support this issue in my free time, right now. I can come back to it when I have more time, aside from a pretty heavy day-job workload and many other various open-source projects I create and support, but a deprecation warning in a dependency of this project isn't my personal primary goal, above all else. This doesn't mean it's not important to me (I use this project and other projects that depend on grpc-node in a book about grpc I wrote, so I'm actually pretty motivated to ensure it all works well.)

To answer the second part of your last comment

what we could've done better to properly convey this deprecation to developers who are using our package

I'm not sure you could reasonably do more. The deprecation warning seems pretty good. @WoLfulus was quick to notice it, and in general, I think it points pretty well to the problem & solution. Short of making PRs to other people's projects that depend on yours (which no one expects,) I'm not sure what you could do. I know that when the gatsby-guy changed his API-shape, or had a really big change that would break other people's stuff, he went around and made tons of PRs to dependant projects, but this is way above-and-beyond normal, and not at all expected.

If all goes as I would like, sometime soon I'll have a bit more time to look at multiple include-paths, the new grpc-node API, and updating the deps for this and other projects so they use the newest upstream grpc-node. Hopefully that all makes sense.

[nicolasnoble]

I see. Thanks!

[konsumer]

I just reread @c0b 's second-to-last comment again. For some reason, in my haste, I didn't notice the last part, before:

I asked you just because you may happen to know more about grpc-node internals...

I'm not sure if I do, I bet @nicolasnoble knows much more about their lib :) If I remember correctly, the old version I used only worked with a single include-path, even though I really wanted it to work with multiple paths, at the time. When I was working on it, I tracked the problem back to protobufjs, which was a dependency at the time (I dunno if it still is, but I think it isn't, now.) The author of that project mentioned that I could use a custom resolver, but it didn't have built-in support for multiple paths. I did a bit of work on it, but couldn't get it working reliably the way I wanted, quickly (basically, exactly like protoc include-path resolution.)

The way I ended up resolving my need for multiple-path, before, was to create a directory structure that just included all the paths, and referenced them from the root. You can do this with symbolic links and also just copy all the files into a single path. It's not ideal for me, either, but until I have enough time to implement multiple include-dirs (which maybe grpc-node now supports?), it'll have to do.

[nicolasnoble]

Right, that's why we have the new proto-loader API, that offers a bit more options when it comes to this problem.

[c0b]

I can possibly try to make a PR to use @grpc/proto-loader ; while I am waiting some response from protobufjs/protobuf.js#1117 the protoset is a different approach to tackle multi *.proto files situation

[konsumer]

Ok, this should work in [email protected], but needs more testing. You can use multiple import paths in CLI by calling it with more -I: grpc-dynamic-gateway -I path/one -I other/path

I'm going to close for now, but feel free to comment more if there are issues.