-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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 null-defaults option #1611
Conversation
@@ -0,0 +1,11 @@ | |||
syntax = "proto2"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be only for proto2
?
does it make sense to test it against proto3
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
--null-defaults
is already the default behavior for proto3 according to @alexander-fenster :
For proto3 optional, I think I got it right in #1584 and #1597 (always assigning them to null).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it's not exactly how it works. Internally missing values might be represented by undefined or null, but the getters pervert his behavior, returning non-null defaults.
Line 313 in 6c4d307
this.defaultValue = this.typeDefault; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for spotting this @castarco. We mostly use static objects (generated by pbjs -t static-module
) and the default values are set to null
there for optional fields; I haven't seen any bad consequences of the field.js
code but I guess we just don't have this use case in our code base. I'll look into this, I believe it should not be hard to fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what I was talking about (proto3
optional fields are internally oneof
members so get null
as their default values):
protobuf.js/cli/targets/static.js
Line 414 in 90afe44
push(escapeName(type.name) + ".prototype" + prop + " = null;"); // do not set default value for oneof members |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm maintaining a typescript code generator ( https://github.com/join-com/protoc-gen-ts/ , quite opinionated, for the specific uses of my company, although it would be usable for others too ), and I had to implement a fix today because of this:
https://github.com/join-com/protoc-gen-ts/pull/37/files#diff-9e3e664b405f7e977363c9421812babe65c6c2ad5f5715f8cab534dd7ee2be78R108
(btw now I realize that the PR did not include the generator changes 😅 , only the results...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
today I learned: you need to link to the specific commit SHA to make GitHub magic work :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this how it is supposed to work? To always use oneOf
fields if we want truly nullable fields in proto3? 😢
This is an example of a generated file:
https://github.com/join-com/protoc-gen-ts/blob/master/tests/__tests__/generated/Test.ts
As in proto3
all fields are "optional" by default, I did not add the optional
parameter to the @Field.d
decorator (example: https://github.com/join-com/protoc-gen-ts/blob/master/tests/__tests__/generated/Test.ts#L342), would it make a difference? (I thought optional
and required
are only applicable to proto2
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this is how this is supposed to work in proto3. Original design: https://github.com/protocolbuffers/protobuf/blob/master/docs/implementing_proto3_presence.md - I just implemented it as is.
The thing is, message fields are always optional. The main reason of having optional primitive fields is to distinguish between integer 0 and "unset" value, same for strings and booleans. Putting them into a hidden oneof
is apparently the easiest non-breaking solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional
fields are supported in proto3 starting from protoc
3.12 (behind the flag) and IIRC 3.15 without the flag.
As far as I can see, this PR only targets the code generation features, but does not take into account JS/TS decorators (or other dynamic loading features), right? |
The typings (JS decorators/TS) already declares that optional field can be null/undefined, so I’m not sure what you mean by "dynamic loading features", but yes this PR only concerns pbjs, not stuff like reflection/custom classes. |
Discussed in #1572 : add an option to pbjs to allow default values of fields to be null instead of the zero value for the type (0 for number, "" for strings)