Support for extensions?

[InfinitiesLoop]

Unless I am doing something wrong (which isn't unlikely), it seems like this library isn't able to parse extended fields. Possibly because doing so with DynamicMessage requires using an ExtensionRegistry:

https://developers.google.com/protocol-buffers/docs/reference/java/com/google/protobuf/ExtensionRegistry

If I'm wrong and it should already work, please let me know. If I'm right that it isn't supported currently, what is the likelihood of it being added somehow? In my case it might even be fine if I had to explicitly pass something into your parse() method, as I know exactly which extensions I need to support.

I'm researching using this library instead of ProtobufJS. I haven't gotten to performance testing with this lib yet, but I'm sure it will outperform ProtobufJS, as it's just plain slow and poorly written IMO. ProtobufJS may be a decent choice for simple cases, especially browser-based ones, but I'm working on a Node project that needs to be simple and perform very well -- and a wrapper around the google c++ library seems like the right approach over re-implementing everything like ProtobufJS has done. I know that Google is adding javascript support in protobuf3, but it does not seem to handle dynamic messages at all, and I'd rather not dynamically load new script in every time my protobufs change at runtime.

So in short, I like this lib, and would really love to use it, but the lack of extension support might block me from using it :( So, hopeful it is something you'd be interested in adding!

Thanks

[fuwaneko]

Pull requests are welcomed. I'll consider adding extensions support but I can't give you any estimates, sorry. You might also consider generating C++ code with protoc and converting it to JS with emscripten.

[InfinitiesLoop]

Thanks for the quick answer. I really like how on top of issues and PRs you are! All the more reason I want to use this lib :)

I don't know whether I'll have time to put together a PR, and I may be able to work around the limitation by using a copy of the proto with the extended fields directly declared. Just means I will need to maintain a separate version of it, but for my scenario that's not the end of the world. I'll let you know if I start working on it as I'll probably have questions.

Thanks!

[InfinitiesLoop]

I looked into this a bit deeper and it's a little more subtle than I first realized.

So, the Google library will already handle looking for Extensions within a DescriptorPool. So if an extension is within the same FileDescriptorSet when the NativeProtobuf instance was created, it should work just fine already. That's great.

The problem is if the extension is on a Google Descriptor. For example, you can define custom Message Options by extending the MessageOptions message within descriptor.proto. It doesn't work with node-protobuf because it parses the FileDescriptorSet without using a descriptor pool: https://github.com/fuwaneko/node-protobuf/blob/master/src/native.cpp#L35

So to get that to work I think it would have to look at the resulting FileDescriptorSet and see if it contains a definition for google.protobuf.FileDescriptorSet. If so, then it means the proto it just parsed could contain an extension for a custom option. It would then have to use the newly parsed FileDescriptorSet and DescriptorPool to re-parse the same descriptor file, using dynamic message to decode the original buffer a second time, only this time it would honor any existing descriptor extensions. Then, users could use their NativeProtobuf instance to parse other descriptors and messages and have their extensions and custom options work. However I'm not sure how viable that is since the result of the 2nd parsing would be a dynamic message.

[InfinitiesLoop]

I think my last comment still applies, but regular extensions don't "already work" like I thought. At first I thought it was just because you didn't pass the DescriptorPool to the DynamicMessageFactory constructor, but I fixed that and it didn't help. In case it's helpful, here's my failed attempt at fixing that:

https://github.com/InfinitiesLoop/node-protobuf/commit/a77216641985a86cc6eec5784dd285e01cd7b943

Also as an aside, I noticed you recreate the factory on every parse/serialize, but they were designed to be reused as it internally caches some stuff. It might be an easy win for performance.

Quote from https://developers.google.com/protocol-buffers/docs/reference/cpp/google.protobuf.dynamic_message#classes-in-this-file

As it turns out, a DynamicMessage needs to construct extra information about its type in order to operate. Most of this information can be shared between all DynamicMessages of the same type. But, caching this information in some sort of global map would be a bad idea, since the cached information for a particular descriptor could outlive the descriptor itself. To avoid this problem, DynamicMessageFactory encapsulates this "cache". All DynamicMessages of the same type created from the same factory will share the same support data. Any Descriptors used with a particular factory must outlive the factory.