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

Tracking: spec-compliant JSON in protobuf.js #481

Closed
lorensr opened this issue Feb 15, 2022 · 7 comments
Closed

Tracking: spec-compliant JSON in protobuf.js #481

lorensr opened this issue Feb 15, 2022 · 7 comments
Labels
enhancement New feature or request

Comments

@lorensr
Copy link
Contributor

lorensr commented Feb 15, 2022

Problem

protobufjs's .toJSON doesn't do proto3-spec-compliant JSON encoding, which means that if we used it, it would either error or be incompatible with the other SDKs, which use Google's libraries, which are spec-compliant.

To work around this, we do this:

https://github.com/lorensr/sdk-node/blob/custom-data-converter/docs/protobuf-libraries.md#current-solution

which is awkward / more setup and code than is ideal.

Solution

Make it easier to use protobufs by changing protobufjs, either by fixing their toJSON implementation or adding a toProto3JSON method. For the status of that, follow:

protobufjs/protobuf.js#1304

@lorensr lorensr added the enhancement New feature or request label Feb 15, 2022
@cretz
Copy link
Member

cretz commented Feb 15, 2022

I think the most challenging-yet-needed thing is making it work from the outside on external protobufjs-generated protos too, not just Temporal ones. Bonus points if this can be a standalone library for anyone using protobufjs (granted it'll be more maintenance and quite the test suite).

I can think of ways to do this. Most are probably similar to the linked issue where a custom toObject is used that understands well known types.

I could also understand punting proto JSON support until the ecosystem gets it together.

@lorensr
Copy link
Contributor Author

lorensr commented Feb 15, 2022

making it work from the outside on external protobufjs-generated protos too

There are different ways that protobufjs generates protos. proto3-json-serializer (and therefore temporal) supports one of the ways. In order to support the other ways, the protobufjs generator would need to be changed to at least include types in generated static classes.

@bergundy
Copy link
Member

This doesn't look like an issue for our repo, instead it's an issue with protobuf.js.
The workaround that we have now using proto3-json-serializer is fine, our users shouldn't care about proto JSON serialization at all, that's handled by the converter.

@cretz
Copy link
Member

cretz commented Feb 15, 2022

our users shouldn't care about proto JSON serialization at all, that's handled by the converter.

From the doc linked in the description, it looks like they do have to care because they have to create protos a certain way (e.g. for returning) and maybe an extra protoc generator or something similar? I'm ok w/ whatever's easiest on users of course. I guess without a type registry and descriptor available there is little choice but making the user care.

@lorensr
Copy link
Contributor Author

lorensr commented Feb 15, 2022

This doesn't look like an issue for our repo

I like to keep issues open just for tracking external issues. But also, this issue is more broad—eg could include discussion of whether to support static generated messages for just binary/protobuf.

@bergundy
Copy link
Member

From the doc linked in the description, it looks like they do have to care because they have to create protos a certain way (e.g. for returning) and maybe an extra protoc generator or something similar? I'm ok w/ whatever's easiest on users of course. I guess without a type registry and descriptor available there is little choice but making the user care.

@lorensr surveyed the alternative protobuf implementations and the solution we came up with is the only one that covers all of the cases.
So yes, the users care to some degree but it doesn't matter if the proto class has a toJSON method or we use an external library for JSON support.

I like to keep issues open just for tracking external issues. But also, this issue is more broad—eg could include discussion of whether to support static generated messages for just binary/protobuf.

So I'd change the description of this issue to reflect that this is its purpose, and possibly close / tag it as (nothing to do now / external issue).

@lorensr lorensr changed the title [Feature Request] Improve protobuf API Tracking: spec-compliant JSON in protobuf.js Feb 15, 2022
@bergundy
Copy link
Member

No need to track as an open issue, it's cluttering up the repo's issues.

@bergundy bergundy closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants