grpc-web icon indicating copy to clipboard operation
grpc-web copied to clipboard

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

Open cola119 opened this issue 5 years ago • 2 comments

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 https://github.com/grpc/grpc-web/pull/448. But I cannot find any spec or definition so I asked. I apologize for not being a contribution

cola119 avatar Jan 27 '20 05:01 cola119

same here. why only message type is build as optional

Hyodori04 avatar Nov 28 '23 10:11 Hyodori04

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 avatar Apr 01 '24 21:04 sampajano

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

https://github.com/grpc/grpc-web/pull/1445

sampajano avatar Jun 25 '24 03:06 sampajano

I'm closing this issue for now since we'd be moving to has_presence() checks in https://github.com/grpc/grpc-web/pull/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!

sampajano avatar Jun 25 '24 03:06 sampajano

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

Hyodori04 avatar Jun 25 '24 04:06 Hyodori04

@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 avatar Jun 25 '24 04:06 sampajano

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

sampajano avatar Jun 25 '24 04:06 sampajano

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

Hyodori04 avatar Jun 25 '24 05:06 Hyodori04

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

sampajano avatar Jun 25 '24 05:06 sampajano