feat: (diregapic)add headers for rest client #871
Conversation
| @@ -156,6 +156,8 @@ export class {{ service.name }}Client { | |||
| } | |||
| if (!opts.fallback) { | |||
| clientHeader.push(`grpc/${this._gaxGrpc.grpcVersion}`); | |||
| } else { | |||
| clientHeader.push(`rest/${this._gaxGrpc.grpcVersion}`); | |||
There was a problem hiding this comment.
Shouldn't the value of this be something else, something that refers to the (language-specific) REST encoding version, instead of grpcVersion? Or does this single number encode both?
[In the latter case, let's at least make a note of that (ideally, changing the name to be more generic, but that might be a more far-reaching change that can handled separately, later, so a comment at least would be good)]
There was a problem hiding this comment.
This is gax-nodejs existing implementation. I think it is worth to think to refactor the grpcClient and isolate fallback or rest from grpc.
But it is not block this task. I will revisit the evaluation. Thanks @vchudnov-g
There was a problem hiding this comment.
Could you generate a comment clarifying that even though the variable is called grpcVersion, it actually captures some non-gRPC info X (where X could be "gax version implementing HTTP" or whatever is an accurate description)
(Someone perusing the code would assume based on the name we're sending the wrong info in the REST case, so it helps to have a comment saying that this is intentional and the variable name is misleading. And then we can fix the name or reorganize the code later.)
| @@ -156,6 +156,8 @@ export class {{ service.name }}Client { | |||
| } | |||
| if (!opts.fallback) { | |||
| clientHeader.push(`grpc/${this._gaxGrpc.grpcVersion}`); | |||
| } else { | |||
| clientHeader.push(`rest/${this._gaxGrpc.grpcVersion}`); | |||
There was a problem hiding this comment.
Now that the value is in clientHeader, it will just automatically show up in the header emitted by the GAPIC (ie no further coding work needed to include the header)? Cool!
| @@ -156,6 +156,8 @@ export class {{ service.name }}Client { | |||
| } | |||
| if (!opts.fallback) { | |||
| clientHeader.push(`grpc/${this._gaxGrpc.grpcVersion}`); | |||
| } else { | |||
| clientHeader.push(`rest/${this._gaxGrpc.grpcVersion}`); | |||
There was a problem hiding this comment.
Could you generate a comment clarifying that even though the variable is called grpcVersion, it actually captures some non-gRPC info X (where X could be "gax version implementing HTTP" or whatever is an accurate description)
(Someone perusing the code would assume based on the name we're sending the wrong info in the REST case, so it helps to have a comment saying that this is intentional and the variable name is misleading. And then we can fix the name or reorganize the code later.)
For REST transport, GAPICs will no longer send the grpc/{@grpc lib version} header value.
Instead,
x-goog-api-clientwill include a new standard value rest/{VVV}. The version here we could reused gax version + "fallback", which is adopted by other headers likegax/{version|}-fallbackandgrpc-web/{version}+fallback.The version value will be update in gax-nodejs PR.