-
Notifications
You must be signed in to change notification settings - Fork 75
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
Support proto2 default values and field presence #716
Conversation
…al, and set initial values on the prototype
… like protoc-gen-es
…prototype properties
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.
Should there be something in the generated code that can fail-fast if the new generated code is inadvertently combined with the old runtime library?
What about the reverse case? What happens if someone inadvertently upgrades to the new runtime library but forgets to re-generate code? It seems like there needs to be some affordance for this since any breakage would probably be pretty subtle and potentially inexplicable to users.
Protobuf bytes fields can have default values. You must be careful not to modify this default value, as it would affect every other instance of the message.
Could the default value be frozen with Object.seal()
? Or perhaps bytes fields could be updated to use a union type that allows Uint8Array
but also some read-only type (with similar read interface and a way to convert to Uint8Array
)?
The change for property typings only applies to scalar and enum fields, not to message fields.
I think this is probably fine, perhaps even expected. (It is certainly similar to message types in protobuf-go, which are very different from other field types since they are pointers.)
@@ -34,7 +34,10 @@ export function isFieldSet( | |||
case "scalar": | |||
if (field.opt || field.req) { | |||
// explicit presence | |||
return target[localName] !== undefined; | |||
return ( | |||
Object.prototype.hasOwnProperty.call(target, localName) && |
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 done this way (vs. target.hasOwnProperty(localName)
) in case somehow the target object has a separate member with that name (like maybe from a Protobuf field named has_own_property
)? Or is this just best practice defensive code in JS for using any functionality inherited from Object
?
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.
It's best practice. eslint (most popular linter in the ecosystem) considers a direct call as an error.
ES2022 adds the static method Object.hasOwn
as a replacement, but we target ES2017 and can't use it yet, most likely similar to many users.
@@ -67,16 +70,45 @@ export function clearField( | |||
target[localName] = {}; | |||
break; | |||
case "enum": | |||
target[localName] = implicitPresence ? field.T.values[0].no : undefined; | |||
if (implicitPresence) { | |||
target[localName] = field.T.values[0].no; |
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.
No way to have scalarZeroValue
compute this for enums? It would be nice to consolidate this case with the one below given how much repetition there is. (Just a thought; not a blocking a comment.)
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 agree that it would be nice to avoid the repetition here, but it would add enough complication for scalarZeroValue
and the ScalarValue type (both parts of the public API), that it wouldn't be a net improvement overall.
That would definitely be nice to have, but it's quite a bit more complex to implement than in Go. A simple function call can throw off tree-shakers, which may either remove the call, or consider it an important side-effect and opt out of tree-shaking for the entire module.
A union type has the disadvantage that it'll apply to every bytes field, and users will have to inspect and convert to declare const bytesOrReadonlyBytes: Uint8Array | ReadonlyArray<number>;
const bytes: Uint8Array = "buffer" in bytesOrReadonlyBytes ? bytesOrReadonlyBytes : new Uint8Array(bytesOrReadonlyBytes); This could be improved with a bespoke class for readonly bytes, or helper function that converts if necessary, but there are pitfalls around assignability that are difficult to predict with TypeScripts complex type system, the dual package hazard for a bespoke class, and it would require significant work to get right, not much different to the runtime-based sealing. If the ecosystem or we come up with a solution for immutable typed arrays, we can bolt it on later. I do not think we should invest the necessary time into this edge case now.
The advantage with protobuf-go is that it's possible to call methods on null pointers. |
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.
LGTM!
This reverts commit 0b0f084.
This reverts commit 0b0f084.
This PR adds support for default values in proto2. For example, consider this simple Protobuf definition:
Note that
string_field
defines a default value, which isn't available in generated code (except through reflection).If you have previously used a "required" field like
bool_field
in the example above, you might also have wondered why we generate an optional propertyboolField?: boolean
.This PR changes the generated code as follows:
Default values
If you access
string_field
from a fresh message, the property will have the default value now. If no default value is specified, the property is initialized with the zero-value for the field type - an empty string in this case,false
for a boolean field, and so on:Field presence
The reason proto2 required fields were previously typed as option in generated code is field presence. Proto2 fields use explicit presence tracking, which means we need to distinguish between the initial value, and a value that is explicitly set. Many Protobuf implementations use getters and setters for this purpose. We use the prototype chain instead.
To determine whether a field is present, we provide the function
isFieldSet
, and the functionclearField
to reset a field to its initial value. Here is a practical example:These new functions provide a unified API to determine field presence for proto2, proto3, and - in the future - editions.
Notes
bytes
fields can have default values. You must be careful not to modify this default value, as it would affect every other instance of the message. This only applies to explicitly set default values, not to zero-values, since aUint8Array
with zero length cannot be mutated.