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

Update TypeScript default values for proto3 syntax #743

Closed
wants to merge 2 commits into from

Conversation

foxdavidj
Copy link
Contributor

This update correctly handles proto3 default value typings

Changes:

  1. All primitive types (and enums) can't be null as they are given default values in proto3. Only messages can be null
  2. Messages default to being "null" in the typings instead of "undefined"

@dcodeIO
Copy link
Member

dcodeIO commented Apr 4, 2017

Optional fields can in fact be undefined or null internally, which is considered as not set (see).

This could still be typed more strictly, of course. Does a Message still satisfy Message$Properties in this case?

Let's say you are creating a message from a plain object that has undefined or null values, then it would just copy these over, effectively setting field values to explicit undefined or null implicitly. Basically, interoperability with plain objects comes at this cost here.

@foxdavidj
Copy link
Contributor Author

How about changing it to <Message>|undefined|null then?

Shouldn't be left as |undefined though, since people may expect typeof foo.bar === 'undefined' to work under all circumstances.

@foxdavidj
Copy link
Contributor Author

Just to clarify, if i have a proto3 file like so:

message Foobar {
  bool value1 = 1;
  string value2 = 2;
}

And i create a message of Foobar type from the following object:

{
  value1: null,
  value2: 'bar'
}

Would value1 have the value of null or false?

@dcodeIO
Copy link
Member

dcodeIO commented Apr 4, 2017

Generated TypeScript definitions evaluate |undefined to public someField?: ... for example. Doesn't that solve it already?

Would value1 have the value of null or false?

If you explicitly .create from an object containing null, it will be null. If you omit the value on creation, accessing the field on the runtime message will evaluate to false because that's the value on the prototype. When using .fromObject, which is the correct approach here, it will not copy null over (evaluate to false from the prototype).

This stuff clearly became too complex, hrm.

@foxdavidj
Copy link
Contributor Author

foxdavidj commented Apr 4, 2017

  1. Yes that "half" works. I've been caught a few times where the value in an |undefined field actually contained null which fails on common tests like typeof message === 'undefined'.

  2. Would you be open to changing the implementation to change the null to the proper default value on primitive types in proto3 messages so it matches the spec?

@dcodeIO
Copy link
Member

dcodeIO commented Apr 4, 2017

which fails on common tests like typeof message === 'undefined'.

The correct test here is message == null (note the use of == instead of ===), which catches exactly null or undefined (JS is fun, isn't it). That's also what generated code uses and != does the exact inverse.

Would you be open to changing the implementation to change the null to a false in proto3 messages so it matches the spec?

This is more a limitation of using a dynamically typed language like javascript plus having the option to simply use plain objects. Definitions use someField?: boolean, which basically is either one of undefined|null|false|true.

@foxdavidj
Copy link
Contributor Author

Wouldn't it be possible to, if the message is proto3, check if the null value in the message is a primitive type and set it to the value it should be? What limitation are you referencing?

@dcodeIO
Copy link
Member

dcodeIO commented Apr 4, 2017

if the message is proto3

Isn't the only difference with proto3 in this regard that all fields are optional? Iirc proto2 optional fields exhibit the same behavior, no matter the syntax - or am I wrong?

What limitation are you referencing?

In a dynamic language, you have to deal with different types. Even when using TypeScript, someField?: boolean still allows entirely different basic types.

What I am trying to do now is to pave the way to remove the ? from fields, see the commit above.

dcodeIO added a commit that referenced this pull request Apr 4, 2017
…ment message fields as non-optional where applicable (ideally used with TS & strictNullChecks), see #743
@dcodeIO
Copy link
Member

dcodeIO commented Apr 4, 2017

This now explicitly types all message fields as non-optional, except for optional messages which are explicitly typed as (MessageType|null). Is this about what you intended?

This should work because constructors and .create now also eliminate undefined and null properties.

@foxdavidj
Copy link
Contributor Author

foxdavidj commented Apr 4, 2017

Yup, this is exactly what I intended! Thanks for taking the time to fix it 😄

@foxdavidj foxdavidj closed this Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants