grpc-loader: add method options in MethodDefinition

[hiepthai]

Add options and parsedOptions into MethodDefinition

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: hiepthai / name: Hiep Thai (a10471b1524f29317ac5cd05c8a413559efe22b4)

[hiepthai]

Can you send a corresponding PR to update this file with the new MethodDefinition fields?

Yeah sure, please check this out https://github.com/grpc/proposal/pull/328

[murgatroid99]

Both of those fields seem to contain the same information; what is the purpose of adding both of them? What does each one provide that is missing from the other one?

I am also still concerned about the types of these fields. any is a very broad type, and I would much prefer to be clear about exactly what is in these fields, especially because this type needs to be re-exported by the gRPC library that it's used with.

[hiepthai]

I updated both MRs with new changes:

• unify options and parsedOptions into options, wont expose raw options anymore
• add MethodOptions interface
• add test case

Waiting for your further feedback, thanks.

[andoshin11]

@murgatroid99 @hiepthai

been waiting for this feature to come out :) What's the status ?

[n0v1]

If it's ok for @hiepthai, I can try to continue work on this PR.

Two questions mainly to @murgatroid99:

1. Regarding "narrowest types possible": If I understand you correctly, e.g. for option deprecated you would like to see just boolean as type. This means if this option is not explicitly set to true inside the proto file it will be returned as false by proto-loader. So we loose the information whether the option was actually set or not. Not set is interpreted the same as explicitly set to false. This is probably fine for most use cases but there might be some which would benefit from an exact representation of the proto file. E.g. for documentation-centric use cases. Is it acceptable to loose this precision or do we want to differenciate between not set/set?
2. What code controls if an option is interpreted (like deprecated and idempotency_level) or not (like (google.api.http))? Is this predefined by protobufjs? I'm asking because I think google.api.http is one of the most widely used options and it could make sense to make it easier to use.

[murgatroid99]

1. My assumption here is that protobuf options are handled the same way as protobuf message fields, meaning that the absence of a field is considered equivalent to setting it to the default value. I don't see any downside of doing that here: there's no meaningful difference between not explicitly deprecated and explicitly not deprecated, and there's no meaningful difference between unspecified idempotency_level and IDEMPOTENCY_UNKNOWN.
2. This distinction is in the official option definitions in descriptor.proto. The options officially defined by protobuf are "interpreted", and all others are "uninterpreted".

[n0v1]

@murgatroid99, I created PR #2711 as a follow-up to this PR which tries to address your comments.

[murgatroid99]

Superseded by #2711.