feat: implemented forceOptionalRepeated=true flag

[dot-i]

No description provided.

[stephenh]

IIRC isn't you're use case taking a JSON body and doing: const message = JSON.parse(...) as MyMessage?

If so, you'll want this on both repeated fields as well as scalars themselves right?

Just thinking it'd be nice to make this more generic/general than very specific to what, without the context of what you're trying to do (if I'm remembering correctly), seems like an esoteric thing to care about.

Maybe matchJsonParseOutput?

[dot-i]

I get the feeling there's some confusion on the use case. This is not to deal with the payload being sent as JSON over the wire, but really the way protobufjs handles your POJOs when performing actual gRPC calls.

For example, when a gRPC payload comes in over Pub/Sub we deserialize this using a deserializer generated by protobufjs (based on the .proto). When an empty array was sent in on the client side, it will actually deserialize this as undefined (this is the default behavior).

The other way around we send payloads to a grpc client, generated by proto-loader and protobufjs. Here the same kind of applies, whether you send in [] or undefined the data over the wire will be the same (since gRPC simply doesn't send empty arrays).

So you see it has nothing to do with JSON.parse() 🙂

Let me know what you propose after reading the above?

[dot-i]

I want to add something important, apparently protobufjs als supports these options:

  {
    arrays:true,
    objects:true,
  }

In which case arrays and maps will actually be set to empty, even if defaults is left to false.

Regardless, it may still make sense to have the option to set repeated fields to undefined or especially fully optional to make it easier to construct objects? Or do you suggest using Type.fromJSON() for those situations?

[stephenh]

Ah okay, sorry, I've had multiple people ask about this JSON-off-the-wire use case, so I assumed that is what you were looking for.

When an empty array was sent in on the client side, it will actually deserialize
this as undefined (this is the default behavior).

I guess as a naive question, why do you prefer undefined? In theory the current non-undefined behavior is less complex to program against b/c instead of checking message.list !== undefined && message.list.length > 0 you can always just check message.list.length > 0.

And, as you pointed out, when grpc sends things over the wire, both empty list and undefined look the same (i.e. aren't sent), so does it matter if we pick empty list or undefined on the ts-proto side to differentiate the two? And, if anything, the proto3 docs specifically recommend using empty list in this scenario.

If this is purely a "we're migrating from protobufjs and we need this for drop-in/backwards compatibility" then I get that, but otherwise it seems like the empty-list behavior is fine? Just trying to understand your use case.

[dot-i]

No, we would indeed actually prefer empty arrays and maps, it's much simpler to deal with as you also describe. But since we were unaware of those arrays: true and objects: true settings we made the incorrect assumption that it's something we simply had to deal with, because of the way protobufjs deserializes (since we do not want to use the global defaults: true setting).

So this does give us a new view on the matter, and we will switch to using those settings. Which also means my PR is probably an over-the-top feature for a use case not many will need.

That is under the new assumption using fromJSON() and/or fromPartial() will make it easy enough to not have to specify empty arrays all over the place when constructing payloads.

[stephenh]

No, we would indeed actually prefer empty arrays and maps, it's much simpler to deal with as you also describe.

Ah cool!

That is under the new assumption using fromJSON() and/or fromPartial() will make it easy enough
to not have to specify empty arrays all over the place when constructing payloads.

Yep, that should be the case, small disclaimer that I'm not looking directly at a test case that does that, so let me know if not.

Which also means my PR is probably an over-the-top feature for a use case not many will need.

Cool, sounds like lets see how the changed protobufjs settings work for you + make sure fromJSON/fromPartial work how you want/expect, and if so we can close this out, afaiu?

Thanks for chatting through it!

[stephenh]

Disclaimer I did more than my fair share of "just copy/paste all of simple.proto and go" when making new integrations, but have since tried to get better about making as-small-as-possible-protos for new integration tests.

Granted, you've already got simple.proto copy/pasted, but can you prune that back and using a more minimal file/schema?

You may just need a single message with a single repeated field at this point. (Maybe more depending on the matchJsonParseOutput use case.)

[gusinacio]

Are there any plans on accepting this PR?

I am mapping an external API and normalizing the returned data using Protobuf. I'm also using AJV for validating the response, with the proto definition (in case, something changes). The problem is that the external API has a lot of optional arrays and for now I need to populate all the optional with empty arrays, so this flag would help me a lot.

[stephenh]

@flametuner no active plans to merge b/c I think the OP found a setting in protobufjs that made its runtime behavior match the ts-proto generated types, i.e. array fields are initialized to [].

For your use case, afaiu you're doing something like:

const theirMessage = someThirdParty.makeApiCall();
const myMessage = ...make this match ts-proto generated type...
MyMessage.encode(myMessage);

?

If so, can you do MyMessage.fromPartial(theirMessage)? I believe that will do the basic "filling in" of "they didn't provide an array, so use [] instead".

Apologies for the late response but going to go ahead and close out this PR for now.