feat(api): add Response.httpVersion() method#39434
feat(api): add Response.httpVersion() method#39434smckee-r7 wants to merge 3 commits intomicrosoft:mainfrom
Conversation
yury-s
left a comment
yury-s
left a comment
There was a problem hiding this comment.
The PR still has too many issues and lacking any tests. Unfortunately, I don't see it converging.
| if (httpVersion === 'h2') | ||
| return 'HTTP/2.0'; | ||
| return this._httpVersion; | ||
| if (httpVersion === 'h3') |
There was a problem hiding this comment.
this does not improve things and just adds confusion, don't change it
| this._getResponseBodyCallback = getResponseBodyCallback; | ||
| this._request._setResponse(this); | ||
| this._httpVersion = httpVersion; | ||
| if (httpVersion) |
There was a problem hiding this comment.
only one client has it ready right away, make it call the new setter and remove the param
| } | ||
|
|
||
| private _onResponse(response: network.Response) { | ||
| private async _onResponse(response: network.Response) { |
There was a problem hiding this comment.
we don't want async event handlers
| async httpVersion(): Promise<string> { | ||
| return this._wrapApiCall(async () => { | ||
| return (await this._channel.httpVersion()).value; | ||
| }, { internal: true }); |
There was a problem hiding this comment.
why do you have this here while it's already defined in the protocol yaml?
There was a problem hiding this comment.
Your 100% right, my bad, I naively dived into this PR before understanding enough and misunderstood the architecture. Once I found the protocol.yml I added the definition there, but forgot to revert this change
I've hopefully resolved your comments and added a basic test. I'm willing get this to a place you guys are happy with but if that's never going to happen, feel free to close the PR. However, not having this feature would be a shame since the only way to assert the httpVersion using HAR recording is currently awkward/heavy |
2 participants
Adds
Response.httpVersion()to get the http version of a given response.Issue: #39310
Previous PR #39309 was closed out with some comments. I've addressed the comments and opened a this new PR. This feature would be extremely useful to reduce bloat/boilerplate for our test suite.