feat: add null-defaults option

[sloonz]

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)

[castarco]

would it be only for proto2?
does it make sense to test it against proto3 too?

[sloonz]

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

[castarco]

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.

protobuf.js/src/field.js

Line 313 in 6c4d307

this.defaultValue = this.typeDefault;

[alexander-fenster]

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.

[alexander-fenster]

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

[castarco]

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

[alexander-fenster]

today I learned: you need to link to the specific commit SHA to make GitHub magic work :)

[castarco]

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)

[alexander-fenster]

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.

[alexander-fenster]

optional fields are supported in proto3 starting from protoc 3.12 (behind the flag) and IIRC 3.15 without the flag.

[castarco]

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?

[sloonz]

The typings (JS decorators/TS) already declares that optional field can be null/undefined, so --null-defaults actually ends up making existing typings more correct.

I’m not sure what you mean by "dynamic loading features", but yes this PR only concerns pbjs, not stuff like reflection/custom classes.