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

Release updated ts-proto-descriptors? #1042

Closed
bhollis opened this issue May 15, 2024 · 4 comments · Fixed by #1043 · May be fixed by VaniHaripriya/data-science-pipelines-tekton#1
Closed

Release updated ts-proto-descriptors? #1042

bhollis opened this issue May 15, 2024 · 4 comments · Fixed by #1043 · May be fixed by VaniHaripriya/data-science-pipelines-tekton#1
Labels

Comments

@bhollis
Copy link
Contributor

bhollis commented May 15, 2024

Since v1.169.0, proto2 optional fields are better supported. Since Google's descriptors.proto is proto2 syntax, this should fix several issues with encoding descriptors using ts-proto-descriptors - for example, since it won't write out optional fields, it's impossible to make an EnumValueDescriptorProto that describes the zero-value option.

I think what's required is to bump the version of ts-proto-descriptors, build it with the latest ts-proto, and publish.

@stephenh
Copy link
Owner

Hi @bhollis ; well I put up #1043 , lets see if you're right 🤞 :-)

@stephenh
Copy link
Owner

Okay, @bhollis I did the basic minimum of bumping ts-proto-descriptors--is that going to do what you want? I'm slightly curious if we should have turned on a proto2-specific build option, but I'm not sure. Thanks!

stephenh pushed a commit that referenced this issue May 16, 2024
# [1.176.0](v1.175.1...v1.176.0) (2024-05-16)

### Features

* Bump ts-proto-descriptors to latest ts-proto. ([#1043](#1043)) ([0b06554](0b06554)), closes [#1042](#1042)
@stephenh
Copy link
Owner

🎉 This issue has been resolved in version 1.176.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@bhollis
Copy link
Contributor Author

bhollis commented May 16, 2024

Thanks for doing that! Unfortunately I was wrong - it wasn't as simple as just releasing the newest version. Even when removing disableProto2Optionals, fields still aren't set when they are at their default value. Here's the change when removing disableProto2Optionals:

    toJSON(message: EnumValueDescriptorProto): unknown {
     const obj: any = {};
-    if (message.name !== "") {
+    if (message.name !== undefined && message.name !== "") {
       obj.name = message.name;
     }
-    if (message.number !== 0) {
+    if (message.number !== undefined && message.number !== 0) {
       obj.number = Math.round(message.number);
     }
     if (message.options !== undefined) {

You can see that the optional fields aren't set when the value is not undefined, when I think they should be.

I can switch over to protobuf-es for this task, so this isn't urgent, but I really prefer ts-proto's approach to generated code in general. This is my first time having to deal with proto2 semantics.

Thanks for the project! If I get some free time I might try to file a PR to fix this, though I have some other things I'd probably want to try first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants