Why does required field become optional in the TS code?

[andness]

I have a (simplified) proto declaration like this:

message Header {
    required uint32 ulId = 1;
    required uint32 ulUnixTimestamp = 2;
    optional uint32 ulVersion = 3;
}

message Message {
    required Header header = 1;
    oneof m {
        PollMessageRequest poll_message_req = 2;
        PollMessageResponse poll_message_resp = 3;
        FirmwareUpgradeRequest firmware_upgrade_req = 4;
        FirmwareUpgradeResponse firmware_upgrade_resp = 5;
    }
}

But the generated typescript code makes Message.header optional:

export interface Message {
  header: Header | undefined;
  ...
}

I was expecting that header would become a normal required field in the TS code, like this:

export interface Message {
  header: Header;
  ...
}

[stephenh]

Hi @andness ; ts-proto primarily targets proto3, which removed requiredas a keyword, so there is not a way to have required sub-messages in proto3.

I assume you're using a proto2 file? I believe there are users of ts-proto using it for proto2, but right some things like this are not implemented. It probably could be if you'd like to poke around and submit a PR. I can link to the relevant lines of code if you're interested. Thanks!

[andness]

Ah, I wasn't aware. I inherited a codebase that uses protobuf so I'm not extremely familiar with it. And correct, it's using proto2. I came to proto-ts after trying protbufjs and finding that it has a huge bug (well, a bug caused by jsdoc apparently) causing it to not generate valid TS code unless you place all your enums last.

If you link to the relevant lines I can certainly have a look!

[andness]

I was able to change it so it does what I want, but I'm having trouble building the code so I had to resort to some hacks to test it with my protobuf definitions. I'm also not able to run the tests, they fail on a missing "pbjs" import. Is there a guide for how to set up the dev environment? (I'm on a Mac)

[boukeversteegh]

I was able to change it so it does what I want, but I'm having trouble building the code so I had to resort to some hacks to test it with my protobuf definitions. I'm also not able to run the tests, they fail on a missing "pbjs" import. Is there a guide for how to set up the dev environment? (I'm on a Mac)

Please have a look at: https://github.com/stephenh/ts-proto#development . Does that help?

PS: the section on dockerized protoc is currently irrelevant, this feature is not working atm

[boukeversteegh]

Docker protoc works again, if you'd like you can have another try getting your code to work.

[anand-paleja]

@stephenh @boukeversteegh I'm using proto3 and also ran into this issue. Is it acceptable to set sub-messages as required if we don't detect the optional keyword ( similar to the behavior of non-scalar values )? If so, it seems we need to add another option to Options.useOptionals for this scenerio. If none of this makes sense, please excuse, as I am not very familiar with protobuf files. I'm willing to create a PR to make this change, once the requirements are clear.

[boukeversteegh]

@anand-paleja it would be great if you could have a look.

First, it might make sense to list the scenarios in which optionals are relevant. Not all of them are handled well at the moment.

• messages:
  • i think messages can be made optional with useOptional=messages
• wrappers - Well known types (which are null able)
  • i think they come out as field: string|undefined rather than field?: string | undefined. the latter would be a great feature
• oneof field groups:
  • members of one fields should all be optional. I think the useopts=messages setting messes this up, but i would need to check. Currently these fields are still required which is not very practical.
• optional fields. This is proto 3? Then it would be nice to support it.

  update: seems it's part of proto3, even though I cannot find documentation for it.

• required this seems like the default behavior unless useOptionals is changed. We're not supporting proto2 but if it can be done, why not.
• scalar - non-message, non-well known type fields:
  • they are required now unless useOptionals is 'all' (or similar), this seems fine from my point of view

What I think we need is a grid of combinations of settings for useOptionals and each use case, and describe the current and desired behavior.

That would make a great feature grid and allow us to fix some of the issues and avoid breaking existing cases. Especially since it can be covered with tests.

I may have missed some cases and I'm curious to hear others thoughts about the desired behavior, and potentially new settings for useOptionals.

[boukeversteegh]

How about turning useOptionals into a set, with options like: all, message, scalar, optional, oneof, wrapper, so users can specify exactly which parts should be rendered as optional?

--ts_proto_opt=useOptionals=message,wrapper,oneof

Each type can be implemented separately.

[anand-paleja]

@boukeversteegh I think those are great ideas. I would suggest adding an additional option to the set: tagged. If set to tagged it would only make fields optional if it is annotated with optional ( with the exception of oneof ). Thoughts?

[boukeversteegh]

@boukeversteegh I think those are great ideas. I would suggest adding an additional option to the set: tagged. If set to tagged it would only make fields optional if it is annotated with optional ( with the exception of oneof ). Thoughts?

yes, I think that's a good addition. Though tag is not part of the protobuf terminology and reminds somewhat of options such as [deprecated], whereas optional is a keyword. So I would go with optional_field / proto_optional to avoid confusion with ts optionals; explicit_optional or just optional - more confusing but simple and still accurate.

[omidh28]

Any updates on this? Super useful

[boukeversteegh]

I think the specs above are clear enough for someone to work on it. As said, each of the options can be implemented as a separate PR.

Does the above draft spec match with what you want @omidh28 ? And which of the options do you need? Any feedback that might help an implementer?

As ts-proto is voluntary like any other os project, it's now just a matter of waiting for someone willing to build it. Anyone is welcome to contribute.

[omidh28]

I'd love to contribute however I suspect that I might not be familiar enough with protobuf.

However it is not clear to me why would we need to add some kind of system like the mentioned tagged or CLI options, because sub-messages that are not explicitly optional do not need to have undefined as part of their type definition. So isn't just correcting the default behavior fixes the problem without adding an extra option? Also it's not really a breaking change.

[anand-paleja]

@boukeversteegh My apologies on not getting back to you on this. We went in a different direction and now do not use this package.

[longility]

@omidh28 I believe it would be a breaking change because what was once optional, is now required from a server side implementation of the contract.