added optional body to delete #439
Conversation
|
MDN has stated that HTTP DELETE method may contain request body. Any reason this PR is not merged? This PR looks good to go apart of merge conflicts. |
|
I updated the PR to resolve the conflict. |
|
As @benedictjohannes suggested, I also added the additional parameters to the global function and abstract Client class, as opposed to only the BaseClient in the last commit. |
lib/http.dart
Outdated
| Future<Response> delete(url, {Map<String, String> headers}) => | ||
| _withClient((client) => client.delete(url, headers: headers)); | ||
| Future<Response> delete(url, | ||
| {Map<String, String> headers, body, Encoding encoding}) => |
There was a problem hiding this comment.
Could you tell me please, what is the exact type definition for the body?
There was a problem hiding this comment.
I can directly send a Map to body, which tries to convert it to (JSON) String, or String. The body field work the same across HTTP verb methods, where it's not included in the GET or OPTION as these verbs doesn't support request body and inside it. Meanwhile, DELETE does support body, hence this pull request.
You can observe that type definition for body isn't also referenced in other methods in this file. Hence my support for this PR
There was a problem hiding this comment.
We lack documentation of the body parameter on the other similar methods too, and the documentation in general isn't amazing, so we should give it an overhaul soon.
No need to do anything special here, it's the same body as all the other body parameters.
There was a problem hiding this comment.
Do you have plans regarding the documentation overhaul? If you share your plans the community might be able to help.
|
Please let this be merged. It was a huge context switch to go all the way from my editor to this PR when I found out |
|
@natebosch can you please review and/or merge this? Its been a while. |
|
We've had this request a few times so far, and each time we push back against the idea of adding an argument that should almost never be used. Especially since there is an existing workaround of using The big thing was it is also a breaking change. Our current situation might push us to adopt this API:
cc @lrhn in case you have any thoughts or feel strongly that we shouldn't do this. I think I'm leaning towards getting this updated and merged. It will take a bit of time to verify that I can get it landed without breaking internal users or the SDK. |
|
@natebosch This does not look like it will break API compatibility since the |
|
The top-level on is definitely non-breaking. The instance method is not something I would consider a breaking change. Nobody else is intended or expected to implement the interface. This is a tool package, intended to be used as-is. Adding it during a major version increment makes it doubly not breaking, so if it's a change we do want, now is definitely the time to do it. I think it's a fine addition, and I'm fine adding it now. |
The interface is specifically designed for extending or implementing. Lines 49 to 52 in d473635
There are at least a few implementations of this class, though I need to check whether any override this method. Please wait to add this until we get things prepped. |
|
Thanks everyone for the contributions and discussion! This will roll out once we are ready to publish the null safety migration for this package. #501 |
The spec discourages but does not forbid a body. Allow a body argument without falling back on the `send` API since some servers may be designed specifically to require it. Closes dart-lang#383, closes dart-lang#206
As requested in issues
#383
and
#206