Skip to content
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

Add better feedback around the types for body argument #652

Open
natebosch opened this issue Dec 24, 2021 · 2 comments
Open

Add better feedback around the types for body argument #652

natebosch opened this issue Dec 24, 2021 · 2 comments
Labels
next-breaking-release Issues that are worth doing but need to wait for a breaking version bump

Comments

@natebosch
Copy link
Member

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.

request.bodyBytes = body.cast<int>();
} else if (body is Map) {
request.bodyFields = body.cast<String, String>();

This is unnecessarily loose and may make the errors harder to diagnose. See error/stack in #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 natebosch added the next-breaking-release Issues that are worth doing but need to wait for a breaking version bump label Dec 24, 2021
@lrhn
Copy link
Member

lrhn commented Jan 3, 2022

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.

@jakemac53
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
next-breaking-release Issues that are worth doing but need to wait for a breaking version bump
Projects
None yet
Development

No branches or pull requests

3 participants