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

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

Merged
merged 2 commits into from
Dec 14, 2021

Conversation

sgielen
Copy link
Contributor

@sgielen sgielen commented Nov 21, 2021

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

@sgielen sgielen changed the title Add the forceOptionals option. Don't write undefined members into the protobuf. feat: Add the forceOptionals option. Don't write undefined members into the protobuf. Nov 21, 2021
Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/main.ts Outdated
${entryWriteSnippet}
});
if (message.${fieldName} !== undefined && Object.keys(message.${fieldName}).length !== 0) {
Object.entries(message.${fieldName}).forEach(([key, value]) => {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 || []?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Collaborator

@webmaster128 webmaster128 Nov 23, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Contributor Author

@sgielen sgielen Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/main.ts Outdated
writer.uint32(${tag}).fork();
for (const v of message.${fieldName}) {
writer.${toReaderCall(field)}(${toNumber}(v));
if (message.${fieldName} !== undefined && message.${fieldName}.length !== 0) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/main.ts Outdated
Comment on lines 597 to 598
(options.useOptionals && isMessage(field) && !isRepeated(field)) ||
(options.forceOptionals && !messageOptions?.mapEntry) ||
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sgielen sgielen Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

src/types.ts Outdated
@@ -251,28 +251,28 @@ export function notDefaultCheck(ctx: Context, field: FieldDescriptorProto, place
const zerothValue = enumProto.value.find((v) => v.number === 0) || enumProto.value[0];
if (options.stringEnums) {
const enumType = messageToTypeName(ctx, field.typeName);
return code`${place} !== ${enumType}.${zerothValue.name}`;
return code`${place} !== undefined && ${place} !== ${enumType}.${zerothValue.name}`;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@sgielen sgielen Nov 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. :)

src/main.ts Outdated Show resolved Hide resolved
@sgielen
Copy link
Contributor Author

sgielen commented Nov 24, 2021

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:

@stephenh
Copy link
Owner

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 sgielen changed the title feat: Add the forceOptionals option. Don't write undefined members into the protobuf. feat: Add useOptionals=all to enable non-field members to be optional. Dec 10, 2021
@sgielen
Copy link
Contributor Author

sgielen commented Dec 10, 2021

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 sgielen force-pushed the feature/allow-all-optional branch 2 times, most recently from 05c8475 to f44a6da Compare December 10, 2021 10:41
@sgielen
Copy link
Contributor Author

sgielen commented Dec 10, 2021

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. :)

Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

${entryWriteSnippet}
});
`);
} else if (packedType(field.type) === undefined) {
chunks.push(code`
const listWriteSnippet = code`
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -32,7 +32,7 @@ export type Options = {
context: boolean;
snakeToCamel: boolean;
forceLong: LongOption;
useOptionals: boolean;
useOptionals: boolean | 'none' | 'messages' | 'all'; // boolean is deprecated
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Owner

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

@stephenh stephenh merged commit e7b70cb into stephenh:main Dec 14, 2021
stephenh pushed a commit that referenced this pull request Dec 14, 2021
# [1.95.0](v1.94.0...v1.95.0) (2021-12-14)

### Features

* Add useOptionals=all to enable non-field members to be optional. ([#402](#402)) ([e7b70cb](e7b70cb))
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.95.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@oyvindwe
Copy link
Collaborator

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
Copy link
Owner

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

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

Successfully merging this pull request may close these issues.

4 participants