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

Document how to resolve imports relative to an import root #1089

Open
silasdavis opened this issue Jul 19, 2018 · 7 comments
Open

Document how to resolve imports relative to an import root #1089

silasdavis opened this issue Jul 19, 2018 · 7 comments

Comments

@silasdavis
Copy link

silasdavis commented Jul 19, 2018

I've spend about an hour trying to work out how to get protobufjs to follow import paths. I'm surprised this isn't a problem more people seem to have, but I had hoped #368 would be the solution, with:

const protobuf = require("protobufjs");
const path = require("path");

protobuf.load({
    root: path.resolve(__dirname, "../protobuf"),
    file: "github.com/hyperledger/burrow/protobuf/exec.proto",
}, function (err, root) {
    if (err)
        throw err;
    // Obtain a message type
    var TxExecution = root.lookupType("exec.BlockHeader");
});

However this fails with 'no such type'. In fact it seems to fail silently on the load since it gives no error if the file is changed to a junk path, so I expect this root/file object is (no longer?) supported. Looking at the typescript it looks like it's expecting a string or list of strings.

Is there a way to provide an import path or paths from which to resolve packages that currently applies? If so I think this would be good material for the main readme and would be happy to add.

@silasdavis
Copy link
Author

Also hitting "protobuf/google/protobuf/descriptor.proto" not found exceptions. It looks like you include that in the repo should it be automatically be substituted?

@dcodeIO
Copy link
Member

dcodeIO commented Jul 19, 2018

The usual way for changing how files are resolved is to override resolvePath. The default behavior differs from how the official implementation does this for no particular but historical reasons, so maybe reintroducing an optional root path makes sense there.

Regarding descriptor.proto: That's a relatively large file that protobuf.js does not need to function, so it isn't included by default. It's still in the repo so it can be packaged by pbjs etc., where necessary. Not sure about substituting by default, as the library might be running in a browser and the file might be located anywhere.

@silasdavis
Copy link
Author

silasdavis commented Jul 19, 2018

Thanks for quick reply. I notice that the grpc package uses an older version of this library which I presume is why the following does seem to work: https://github.com/silasdavis/burrow/blob/protobuf.js/burrowjs/index.js

Regarding overriding resolvePath I did see that in the code and is useful to have, but having to override a function well beneath the API service level is not something most people would want to do lightly and requires understanding exactly how it is used. Being able to resolve imports from a single import path is such a common and standard thing to be able to need to do when depending on proto files from other languages that it would be nice to have it as a first class functionality. If it's easy to have multiple import paths then you can have the same modality as with the official compiler protoc -I import_path_one -I import_path_two, but I could live with just having resolution from a single root and not having to maintain my own resolve path.

It would also be nice if protobuf.load passed an error to the callback if it doesn't like its argument.

@crdev
Copy link

crdev commented Feb 27, 2019

👍

@sirianni
Copy link

sirianni commented May 12, 2020

I'm in the process of spending hours myself figuring this out. If I have all my protos defined in a subdirectory called proto, then what should I set Root.resolvePath to in order to make this work?

I tried this and it doesn't seem to even be calling my resolvePath function

protobuf.Root.resolvePath = (origin, target) => {
  console.log(`origin: ${origin}, target: ${target}`);
  return 'proto/' + target;
}

const protoRoot = protobuf.loadSync('proto/opencensus/proto/agent/metrics/v1/metrics_service.proto');

EDIT: I was able to get it working.

const root = new protobuf.Root();
root.resolvePath = (origin, target) => {
  console.log(`origin: ${origin}, target: ${target}`);
  return 'proto/' + target;
}

root.loadSync('opencensus/proto/agent/metrics/v1/metrics_service.proto');

@peternann
Copy link

I have a complex pair of inter-dependent proto folders.
With protoc I just point 2 include paths at the top of each tree, reference any .proto file relatively to one of those, and it works.

Very surprised to find there was no "multiple Includes" feature with protobuf.js to achieve an equivalent outcome.

It's a real gotcha and surprise that you have something working with protoc, and you try to use the same thing with protobuf.js and hit what seems to be a dead-end.

It would be awesome if (and expected that) protobuf.js equivalently supported the standard features of protoc here - i.e. Multiple include paths.

@Daij-Djan
Copy link

thank god for this thread or I'd never have figured out how to open .proto files.
lets document in on main page plz :)

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

No branches or pull requests

6 participants