http
http copied to clipboard
Add better feedback around the types for `body` argument
The doc comment says that the body "can be a String, a List<int> or a Map<String, String>." however we allow any List or Map and then enforce the value types with a cast.
https://github.com/dart-lang/http/blob/778174bca2c13becd88ef3353309190b1e8b9479/lib/src/base_client.dart#L85-L87
This is unnecessarily loose and may make the errors harder to diagnose. See error/stack in https://github.com/dart-lang/http/issues/644
I think we should consider making this more strict - require a List<String> instead of any List that happens to contain only String values, and same for Map. If we decide to do that we should only change it in the next major version.
We could also consider treating it as non-breaking to add an assert to help catch this kind of thing in tests. There are only a few existing cases that need to get fixed internally.
WDYT @lrhn @jakemac53 @kevmoo
One possible reason for the code is that it predates Dart 2.0, so people writing literals like {"foo":"bar"} would not get type inference, and the code would allow <dynamic, dynamic>{"foo": "bar"} to support that.
The Dart 2.0 migration then chose to allow the existing inputs, and only cast the contents. That was the "safe" and permissive way to migrate, even though new code would likely get type inference on literal arguments, and wouldn't need the cast.
I'd be all for doing stricter checks.
Hmm ya I be a bit concerned about cases today where people are passing in a dynamic map that happens to contain only string values. In general I would be all for the static check, but I wouldn't do a breaking change in this package for it.
I think adding an additional assert, or possibly a try/catch around the cast with a better message, would be good for usability.