protocolbuffers / protobuf-javascript

BSD 3-Clause "New" or "Revised" License
365 stars 67 forks source link

JavaScript: Package and type metadata in generated files #89

Open cpx86 opened 7 years ago

cpx86 commented 7 years ago

I need to be able to get the package name and type name for any given Protobuf object, but I can't find this metadata anywhere in the code. In e.g. the C# plugin, this is available in the ProtosReflection.FileDescriptor property.

Am I missing something or is this functionality just not available?

cpx86 commented 7 years ago

Dug into the generated JS and the JS generator itself and can verify it doesn't generate any metadata whatsoever (except the displayName for debug purposes).

I'm very interested in seeing improvements done to the JavaScript generator. I cloned the repo and was easily able to add these, so I'd be willing to take this on.

xfxyjwf commented 7 years ago

@cpx86 Thanks for taking this on, but please confirm with @acozzette or @haberman before working on the implementation. They know more context about why the feature is missing and can guide the API design. It's also possible that we already have the feature in Google but it's just not opensourced.

cpx86 commented 7 years ago

Absolutely I won't dig into it deeper until there's been some discussion about the background and details.

Somewhat related: In the end it would also be good to have a generator that outputs ES6 code - e.g. classes instead of prototypes, properties instead of get/set functions. Separating the protobuf data objects from their serialization functions would also make sense IMO.

acozzette commented 7 years ago

+@updogliu who is also working on Javascript

@cpx86 Do you want to give us a quick summary of how you were thinking of implementing this?

cpx86 commented 7 years ago

@acozzette I think the most useful option would be to basically to expose all of the descriptors, starting with the file descriptor, in the generated code, similar to how the C# generator does it. So you could do something like:

let messages = require('./messages_pb')
let fd = messages.Reflection.FileDescriptor
console.log('package name', messages.Reflection.FileDescriptor.Package)
for(mt in fd.MessageTypes) {
    console.log('message type', mt.Name)
}
for(et in fd.EnumTypes) {
    console.log('enum type', et.Name)
}
for(s in fd.Services) {
    console.log('service', s.Name)
}

So quite a big feature, but a lot of the design can probably be copied from C# and work well in JS.

cpx86 commented 7 years ago

There would also need to be a way to include the type name as a property on the actual message objects. If the generator generated ES6 classes this would be possible to get using message.constructor.name, although that wouldn't work if the message type name is a protected keyword. Adding a property to the prototype wouldn't work either if the message has a field with the same name. So I guess the only safe way to include this information would be to add it to a property on the constructor - e.g.:

// generated code
proto.mypackage.MyMessage.typeName = 'MyMessage';

// in client code
let myMessage = new mypackage.MyMessage();
console.log('type name', myMessage.constructor.typeName);

Another option could be to reference the previously suggested descriptors to the message. This might be the cleanest option. e.g. something like:

let myMessage = new mypackage.MyMessage();
console.log('type name', myMessage.getDescriptor().name);
haberman commented 7 years ago

It might be possible to add this functionality. However there are a couple major constraints based on how we use protobuf JavaScript inside Google:

  1. when using an optimizing JavaScript compiler, any extra code for descriptors must get completely stripped away (dead-code eliminated). In other words, it must cause no code size overhead for people who aren't using the feature.

  2. it must be possible to disable descriptors completely for people who don't want them.

The easiest way to achieve these goals would be to generate the descriptor-related code into separate files (like _pb.desc.js perhaps). Then people who want descriptors would generate both regular generated code (_pb.js) and the descriptor code (_pb.desc.js). To achieve goal (1), we can make sure that _pb.js files never depend on _pb.desc.js files. To achieve goal (2), people who don't want descriptors can just decline to generate the _pb.desc.js files.

cpx86 commented 7 years ago
  1. You mean using e.g. Closure Compiler?
  2. Would a generator option be the correct way here?
cpx86 commented 7 years ago

To achieve goal (1), we can make sure that _pb.js files never depend on _pb.desc.js files

Would it be OK for the _pb.desc.js file to depend on the _pb.js file and extend the generated message classes with members for getting the message descriptor?

haberman commented 7 years ago

For (1): yes, exactly. Closure Compiler is what we use internally at Google.

For (2): ideally we would stay away from generator options. It's cleaner if we can just generate separate files, because then anyone who wants to generate the extra files can. If someone else generated the _pb.js but didn't enable the option, you're a little bit stuck.

Yes, making _pb.desc.js depend on _pb.js should be fine; that's probably the best way to do it. Let me just run this by some more people inside Google to make sure this plan seems right to everyone.

cpx86 commented 7 years ago

Reg 2. - My thought was to add an off-by-default generator option that would create a separate file. That way people who aren't interested in having descriptors won't suddenly get a bunch of _pb.desc.js files in their source code that they don't care about.

Given that all of the above checks out, how would you guys like going about implementing this? Since I have somewhat of a vested interest in getting this done as soon as possible (I'm currently completely blocked in porting an OSS library to JS), I'd be more than happy to pitch in. It would also be a lot of fun 😄

rogeralsing commented 7 years ago

How does Protobuf JS handle the protobuf Any type? If Protobuf JS supports Any, then all of this must already be supported under the covers. And if not, then Protobuf JS isn't protobuf compatible.

In out project we use the exact same concept as the any type, we fetch the type name and serialize the messages into an envelope. The reason we don't use Any directly is that we want to optimize a few things by re-using strings in a lookup table etc, a form of light weight compression if you will. All of this is beside the point, but TLDR, we want to use the same features as the Any type support needs.

cpx86 commented 7 years ago

I ran some experiments and when I tried to serialize a message type with Any it threw the error TypeError: message.getTypeUrl is not a function at proto.google.protobuf.Any.serializeBinaryToWriter. The current JS generator does not create this function on the message types and I can't find any reference to it in the JS library other than in the files generated from any.proto.

@haberman Is the Any functionality in JS incomplete, or am I missing something?

haberman commented 7 years ago

If Protobuf JS supports Any, then all of this must already be supported under the covers.

By "all of this" I assume you mean descriptors and other metadata.

The Any support in Protobuf JavaScript does not use descriptors or any metadata. It relies on the user explicitly specifying the name in the pack/unpack calls. For sample usage see: https://github.com/google/protobuf/blob/651ba62ab5a9c35e157380e2fd89cf77febb47b9/js/binary/proto_test.js#L646

The current JS generator does not create [getTypeUrl()] on the message types and I can't find any reference to it in the JS library other than in the files generated from any.proto.

It sounds like you are trying to use Any.serializeBinaryToWriter with a type other than Any itself. This isn't a generic function, it is only meant for Any messages. To pack/unpack another message type into an Any, use pack and unpack as in the example: https://github.com/google/protobuf/blob/651ba62ab5a9c35e157380e2fd89cf77febb47b9/js/binary/proto_test.js#L646

If we did have true descriptors, the Any functionality could be made more convenient (for the cases where descriptors were available).

janhancic commented 7 years ago

Would this allow me to use custom options in JS? I want to do something like this for my enums:

import "google/protobuf/descriptor.proto";
extend google.protobuf.EnumValueOptions {
  string display_name = 50005;
}
enum MyEnum {
    MYENUM_UNKNOWN = 0;
    MYENUM_ONE = 1 [(display_name) = "My enum one"];
}

But I can't since the custom options don't get added to the generated JS (they do for Python on the other hand). Let me know so I can open a new issue if this is something else. If it is though, then I too would be very interested in support for this.

tobowers commented 6 years ago

I don't suppose this went anywhere, eh? I'm just running into this problem with "any" now.

xfxyjwf commented 6 years ago

Reflection support in JS is not on our roadmap. Support for Any is actively being worked on though. @TeBoring

ZacVawterKodiak commented 6 years ago

Would really love the js reflection support (in separate files is fine)..

chrisplusplus commented 6 years ago

I ran some experiments and when I tried to serialize a message type with Any it threw the error TypeError: message.getTypeUrl is not a function at proto.google.protobuf.Any.serializeBinaryToWriter. The current JS generator does not create this function on the message types and I can't find any reference to it in the JS library other than in the files generated from any.proto.

@haberman Is the Any functionality in JS incomplete, or am I missing something?

Is there a solution to this? I'm getting the same error

gravypod commented 4 years ago

Hey, just wanted to leave a comment here saying that this would be extremely helpful for things like automatic form generation. We have some use cases where we want to build internal tooling (and the UX/UI isn't too important) but getting things implemented just by adding fields into a proto would be extremely helpful. @cpx86 if you had implemented any code around this I'd be more than happy to rebase it if that would get it merged.

axedre commented 3 years ago

I need to be able to get the package name and type name for any given Protobuf object, but I can't find this metadata anywhere in the code. In e.g. the C# plugin, this is available in the ProtosReflection.FileDescriptor property.

Am I missing something or is this functionality just not available?

Stumbling across this issue / feature request close to 4 years after it was opened, since I would love to be able to generate _pb.desc.js (better yet - _pb.desc.d.ts) files as well. Wondering if there's a roadmap for this, or a sensible workaround. @cpx86 you mentioned you were able to implement it by forking the repository?

perezd commented 3 years ago

Strategically we're rethinking how the JavaScript implementation of Protobufs should work and we'll keep this feedback in mind going forward, but I'm unable to comment on direct improvements to the existing implementations at this moment in time. Thanks for checking in!

ideasculptor commented 3 years ago

Add me to the list of people who wants to be able to do the same things in javascript that we already do in other languages, such as Golang. Discovering the type of a protobuf dynamically seems like the bare minimum of functionality people might want. Whether google is using that functionality inside the company or not, if you are building something to be compatible with protobufs elsewhere, it seems like the functionality really ought to be made available - especially when you have 3rd parties volunteering to take on the work. If "google needs the feature internally" is the minimum bar that any feature has to meet in order to be added to the js implementation WHEN THE FEATURE ALREADY EXISTS IN THE OFFICIAL LANGUAGE DESCRIPTION, then how is anyone supposed to make use of protobuf support in JS? We just sit on our hands and wait for the mighty google to need reflection? Good grief - you've got implementations of the same funcitonality in how many other languages sitting right here? How hard could it be to add just for the sake of correctness and encouraging adoption of protobufs?

Why tout protobufs as a multi-language solution if, in fact, the intent is not to fully support protobufs in otherwise supported languages? It would be one thing if this were a feature that is never requested, and if the requests were reasonably recent, but here we are after years of waiting, with still no resolution in sight. Is google trying to cause a fork of their protobuf library and compiler for JS?

dibenede commented 2 years ago

Reflection APIs are intentionally not provided as part of the main product. If they were provided in the future, we would need to provide it as a separate parallel set of artifacts so that bundlers could easily drop it.

michiel-asana commented 1 year ago

Pretty interested in this. We are currently forced to carry clunky interfaces, because we can't use Any.pack without being able to access the fullname of the message. Even if it just is adding a static field on Message subclasses that give the fullname would be sufficient