http icon indicating copy to clipboard operation
http copied to clipboard

Add better feedback around the types for `body` argument

Open natebosch opened this issue 3 years ago • 2 comments

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

natebosch avatar Dec 24 '21 00:12 natebosch

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.

lrhn avatar Jan 03 '22 11:01 lrhn

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.

jakemac53 avatar Jan 04 '22 15:01 jakemac53