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

Question: Why message types are treated as optional property? (TypeScript) #715

Closed
cola119 opened this issue Jan 27, 2020 · 9 comments
Closed

Comments

@cola119
Copy link

cola119 commented Jan 27, 2020

I wonder why message type objects are treated as optional. The generator provides the type of AsObject as below:
https://github.com/grpc/grpc-web/blob/master/javascript/net/grpc/web/grpc_generator.cc#L952-L970
On line 962, it checks if a field type is TYPE_MESSAGE and if TYPE_MESSAGE its fields are considered to be optional. As a result, the following type definition is generated:

export namespace someObject {
  export type AsObject = {
    id: string,
    hoge?: hogeObject.AsObject,
  }
}

But default values will be set automatically if the message does not contain any data.
ref. https://developers.google.com/protocol-buffers/docs/proto3#default
So I think it seems to be tedious treating as optional properties and rather it should be as a normal object type (not optional).

This generation method was implemented in #448. But I cannot find any spec or definition so I asked. I apologize for not being a contribution

@Hyodori04
Copy link

same here. why only message type is build as optional

@sampajano
Copy link
Collaborator

Hi! Thanks for reaching out!

I think the reason could probably be explained by this following page:
https://github.com/protocolbuffers/protobuf-javascript/blob/main/docs/index.md#singular-scalar-fields-proto3

Where in Proto3, Primitive types only has getFoo() and setFoo() methods, whereas for Message types, an additional hasFoo() method would be available — meaning the messages are optionally available.

This would probably also translated to how toObject behaves in JS Protobuf. But I haven't personally verified it.

@sampajano
Copy link
Collaborator

FYI - I'm guessing the following PR is related :)

#1445

@sampajano
Copy link
Collaborator

I'm closing this issue for now since we'd be moving to has_presence() checks in #1445 which should be specifically address the "hasFoo()" semantic — which I suppose would be accurate in deciding whether a "?" should be added.

Please feel free to re-open if further confusion remains. Thanks!

@Hyodori04
Copy link

It's really nice to hear fixing. When are you going to release this fix?

@sampajano
Copy link
Collaborator

@Hyodori04 Hi :) I'm actually not 100% sure it'll be fixed.. It's just that we're updating how the "?" check is done so i'm supposing that this could be impacted (hopefully in a positive way).. :)

I'm hopefully cutting a new release soon. But in the meantime let me create a test binary so if you could help test it out it would be nice to know if it's actually fixed or not 😃

@sampajano
Copy link
Collaborator

sampajano commented Jun 25, 2024

FYI I've tried making some new test binaries but i think our workflow file is outdated :)

https://github.com/grpc/grpc-web/actions

Will need to look into this later (after july 4th). sorry for the delay :)

@Hyodori04
Copy link

@sampajano Hi, I can test it for you when the test binary is ready. Please leave a comment when it's updated, and I'll check it out.

@sampajano
Copy link
Collaborator

@Hyodori04 Thank you very much!! Will do after July 4th vacation!

(There's various random build failures right now i probably cannot fix them all before i goes on vacation now.. 😅)

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

No branches or pull requests

4 participants