Consider tightening arguments that are dynamic

[natebosch]

The dynamic url which may be either a String or Uri is not an idiomatic pattern in Dart 2. You lose static checking and are forced to read the doc comment to get help with usage that is normally provided in your IDE. The migration would be easy, shift the Uri.parse call to the call site instead of inside the library code.

The downside here is that there is potentially a lot of code to migrate since this is such a widely used package. That is mitigated somewhat by the fact that you'll get static checking for the migration in the vast majority of cases, only dynamic invocations would be runtime failures.

[jamiewest]

Maybe annotate the old method signature as obsolete and add a new one that only takes a Uri.

[natebosch]

Adding an alternative would be the typical upgrade route. In this case it would likely leave us with less desirable naming and wouldn't save much cost in the migration. The vast majority of this will be caught statically and the migration is trivial so it doesn't really need an long rollout.

[jakemac53]

Another alternative that came up today would be to add an extension on the Uri class which adds all of these methods, something like:

extension Http on Uri {
  Future<Response> get({Map<String, String> headers}) =>  get(this, headers: headers);
}

This allows for static safety through this api, without conflicting with the existing api.

To accomodate strings we could add a toUri extension on String:

extension Uri on String {
  Uri get toUri => Uri.parse(this);
}

So final usage is like var response = await 'https://www.google.com'.toUri.get() etc.

[Jonas-Sander]

Hm, to be honest I really like the convenience with using a String as an URL.
I never had a problem with that.
Also it may be a bit more convoluted for newcomers to the language instead of using a simple String. On the other hand they may be confused today because it's dynamic so they don't know from the IDE that they should/could use a String - they may only assume that.

I think both approaches have pros and cons. In my opinion the real fix would be to wait until there are better ways to express unions - with the current dynamic approach there may not even be breaking changes when unions do finally happen in the future (dynamic may be replaced with String | Uri which should break no code anywhere).

In the end I guess you'll already have made up your mind so idk. Just seems like a big breaking change for me with some amount of "unnecessary" cruft work (adding Uri.parse everywhere).

[jakemac53]

Ya, it is a fairly impactful breaking change, but the migration is very mechanical and you get static errors for all the places that need to be fixed up.

We found a lot of evidence when examining existing code that the existing api was very confusing to folks - many appeared to think it only accepts a String. This lead to a lot of sub-optimal code (several instances of code converting a Uri to a String just to pass it into these functions for instance). Also a lot of people attempting to do their own uri escaping in strings, which if they just used the Uri class to build it up they would not have had to do, etc (not to mention this was never done exhaustively). People were also building up uris in very inconsistent and fragile ways. This won't prevent them from doing that (a common migration will likely involve a simple conversion to use Uri.parse), but generally we actually want people to be building up uris using the Uri class.

So, those are some of the motivations for this change.

[Jonas-Sander]

Thank you for clarifying:)