-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Standardize the layout of the http _response property in output types #10887
Comments
Thanks for the feedback! We are routing this to the appropriate team for follow-up. cc @wmengmsft, @MehaKaushik, @shurd, @anfeldma-ms |
Related to discussion in #10602 (comment) There was debate about whether it should be typed at all, but if it is typed, I agree that EDIT: Sorry, that also makes parsedHeaders a string -> any dictionary, which I think is also fine as an escape hatch typing, but is in fact different from your proposal. |
Widening to I like |
Stepping around the "is this a breaking change?" issue, I think ultimately it's better to get out of the business of typing/guaranteeing intermediate request pipeline goop. Right now I would rather standardize around While we're at it, we should revisit what we want to expose on RestError. Today it exposes the full response object, including parsedBody, parsedHeaders, etc. Whatever we decide for |
Existing consistencies
There are a few inconsistencies regarding how do we define the
_response
property in our output types. Those inconsistencies are discussed below:Not every package supports the
_response
propertyList of packages that do support the
_response
property:List of packages that do not:
The shape of the
_response
property varies between packagesOn one hand, App Configuration has the following type that is extended by output types:
On the other hand, all other packages mirror the generated response type in some sense and change the type that is being intersected with to be the corresponding one from the convenience layer alongside other convenience changes. For instance, Tables has the following generated response type (located at
sdk/tables/azure-tables/src/generated/models/index.ts
):and in the convenience layer, it has the following corresponding response type (located at
sdk/tables/azure-tables/src/models.ts
):Which looks similar to the App Configuration's one but has the extra property
parsedBody
that is typed using the generated typeTableQueryResponse
which is fully static in the sense that it does not have anyany
members.What is the best shape for
_response
anyway?From the JS SDK weekly team meeting on 8/26/2020, the proposed shape is to mirror the generated response types but if the
parsedBody
property exist, type it asany
, so the Tables'sListTableItemsResponse
would become:How to move forward?
I suggest that the
_response
property should be added to all output types in all packages according to the approach we agreed on. Furthermore, I suggest to change the typing ofparsedBody
property in packages that already support the_response
property to beany
and no longer expose the generated type if it is not needed elsewhere.It is yet to be checked if there is any customer who relies on the type of
parsedBody
. I will reach out to the .Net SDK team asking for such scenarios if any.The text was updated successfully, but these errors were encountered: