feat: Add useOptionals=all to enable non-field members to be optional.

[sgielen]

This PR also adds a test to verify writing empty objects and reading them back.

[stephenh]

Looks great! A few questions but otherwise lgtm direction wise.

[stephenh]

I would if we could make this Object.entries(message.foo || []).forEach. I know it's slightly less efficient, in terms of making a throw away [] and lambda if the field is not set, but I'm thinking that @webmaster128 just did several PRs that a lot of explicit message.field !== undefined && ... checks to reduce the code size ouptut.

So, I suppose it's a trade-off, do you prefer less code w/a few extra allocations, or more code w/a few less allocations?

I suppose in an ideal world, ts-proto would let the user pick, but we already have a ton of flags, so dunno, I think I'd prefer to just pick one.

@sgielen wdyt about || []?

[webmaster128]

I'd not worry about performance too much. JIT engines run this stuff amazingly fast these days. The better the optimizer understands the intend of the code, the better.

I'd use Object.entries(message.foo ?? {}) to

1. Only use the fallback in case of null and undefined and not bugs like "", 0 or false,
2. Always have an onject as input to Object.entries(

[webmaster128]

But this extra undefined check is only needed in case of forceOptionals = true? So maybe the code output should only be generated in that case. Same in the diff below.

[sgielen]

I can switch to || [] and || {} no problem.

I'll respond to the "extra undefined check" comment in another thread on this PR. :)

[sgielen]

Although - if we switch to || {}, we do put empty maps on the wire (as in your other comment about lists), while if we keep if (.. !== undefined), we don't put the map on the wire. Shouldn't it be preferred not to put the map on the wire if it's undefined or empty? What do you think?

[sgielen]

Derp, nevermind -- an empty map doesn't actually cause any writer calls, so there's no on-wire difference between an empty map and not writing the map at all. So, this is good to go.

[stephenh]

Unlike the other spot where I suggested || [], it does seem nice here to keep the tag completely off the wire if there are no elements in the collection.

[stephenh]

Per discussion, I'm good with removing options.forceOptionals and just remove the isMessage / !isRepeated from the existing check.

Also I assume !...mapEntry is in here just b/c there are some nuances to making it optional that you didn't want to tackle in this PR? If so, maybe just add an in-source comment/todo that it's for later.

[sgielen]

Yes, otherwise I had to assert that the Map entry key is set in various places, which seems silly as the Map entry key is mandatory. I'll explain it a bit better in the comment.

[stephenh]

Hm, maybe we could do:

const maybeNotUndefined = options.useOptionals ? `${place} !== undefined && ` : "";

At the start of this method, and then in these places do:

return code`${maybeNotUndefined} ${place} !== ${enumType}.${zerothValue.name}`;

So that users that don't have useOptionals set won't have the unnecessary !== undefined in their output.

Granted, it doesn't hurt, but I think I'd like to keep it out if possible.

[sgielen]

I actually had problems with this before implementing force/useOptionals. Suppose that you have a message FooRequest with a repeated int64 list = 0; (remember you can't mark repeateds as optional), and an RPC SendFoo(FooRequest). This code, intended to send an empty list (as with Go semantics), compiles but then triggers a runtime error:

SendFoo({} as FooRequest)

The error triggered is quite deep inside ts-proto and reads something like "list is not iterable", regardless of the value of force/useOptionals. IMHO, it is a small price to pay to always do the undefined check to allow this code to work normally.

If you strongly prefer to only enable this behaviour with force/useOptionals enabled then I can apply your suggestion, but personally I would strongly prefer not to.

[stephenh]

Hm, so for SendFoo({} as FooRequest), I would have expected that to go through Foo.fromPartial({}) which would have ended up doing message.list = (object.list ?? []).map(e => e). I.e. like in simple.ts:

    message.coins = (object.coins ?? []).map((e) => e);

But it's hard to say without seeing the stack trace...

I definitely get why this change is necessary with forceOptionals, b/c undefined are now valid values of these fields; but w/o that, the field shouldn't be undefined, and if it's not, that seems like the bug I'd rather fix. Granted, these is something to be said for defensive coding, but code / output size is already a concern, so, dunno, yeah, would appreciate doing maybeNotUndefined here, at least for this PR, and we can follow up on "list is not iterable" as a separate PR/bug.

Unless I'm missing something about "the field should never be undefined" in the SendFoo({}) use case that means it's not going through fromPartial and having the ?? [] applied that way?

[sgielen]

OK, I'll revert to only add the undefined-check if useOptionals is set and make a separate issue for the [] being undefined in a partial object, so we can discuss it further there. :)

[sgielen]

Rebased and updated.

I've kept the flag forceOptionals since it looks like that's where consensus is heading.

There's still two open points AFAIK:

• Allow members to be undefined even if forceOptionals is not enabled? feat: Add useOptionals=all to enable non-field members to be optional. #402 (comment)
• Rerun codegen + add the diff to this PR before merging.

[stephenh]

Rerun codegen + add the diff to this PR before merging.

Cool, yeah, I think now is a good time to do that.

I've kept the flag forceOptionals since it looks like that's where consensus is heading.

I'd like to keep just the existing useOptionals, b/c we already have a large proliferation of flags. Instead of just true/false, about how doing useOptionals=...:

• false = current/default behavior
• messages = the current useOptionals=true behavior
• all = the forceOptional behavior you're implementing here
• true = outputs a warning that true is deprecating, but keeps going using the messages behavior

I think this should let us achieve no breaking changes for current useOptionals=true users, but also set us up for useOptionals=messages/all going forward as I think nice/valid long-term options for users to choose which they want.

Wdyt/sgty?

[sgielen]

Rerun codegen + add the diff to this PR before merging.

Cool, yeah, I think now is a good time to do that.

I've kept the flag forceOptionals since it looks like that's where consensus is heading.

I'd like to keep just the existing useOptionals, b/c we already have a large proliferation of flags. Instead of just true/false, about how doing useOptionals=...:

• false = current/default behavior
• messages = the current useOptionals=true behavior
• all = the forceOptional behavior you're implementing here
• true = outputs a warning that true is deprecating, but keeps going using the messages behavior

I think this should let us achieve no breaking changes for current useOptionals=true users, but also set us up for useOptionals=messages/all going forward as I think nice/valid long-term options for users to choose which they want.

Wdyt/sgty?

Yes, sounds good to me! I'll make a tiny change and make useOptionals: boolean | 'none' | 'messages' | 'all' and make none the default so that in a future version, we can remove the boolean option altogether. If that's OK with you?

[sgielen]

Both changes now implemented. This also means that rerunning codegen is not necessary: the codegen output is now equal if useOptionals is not set to messages or all.

I think this is ready for final review. :)

[stephenh]

@sgielen looks great! Thanks for iterations on this.

If you want to also update the readme, that'd be great, but that's also fine to do as a follow up/later.

[stephenh]

Cool, this is a nice way of conditionally adding this behavior.

[stephenh]

One option would be to make this type:

useOptionals: ['messages' | 'primitives'];

And then around in here:

https://github.com/stephenh/ts-proto/blob/main/src/options.ts#L122

Do a translation of true -> [messages, primitives], false / none -> [].

Hm, I guess parseParameter is not fancy enough to like detect , and do a .split(,). It probably could though, and seems like a reusable thing.

If you want to add this as part of this PR, that'd be great, but also fine to merge as-is.

[stephenh]

@sgielen I went ahead and rebased this due to a minor import conflict and am going to merge. Thanks for the PR!

[stephenh]

🎉 This PR is included in version 1.95.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[oyvindwe]

A little late, but great to see this PR, as we had a local patch to do the same thing (we parse JSON responses from our gRPC servers directly into types generated by ts-proto to avoid including the fromPartial methods for hundreds of messages in our webapp).

However, README.markdown is not updated with useOptionals=messages and useOptionals=all, so it was a bit hard to discover this feature.

[stephenh]

@oyvindwe thanks for the call out; I've updated the useOptionals docs in the readme, feel free to suggest further improvements. Thanks!