Update Client interface #56

donny-dont · 2017-02-17T06:48:22Z

This updates the Client interface exposed and removes the extensions of Request and Response that are no longer applicable.

kevmoo · 2017-02-17T16:47:17Z

lib/http.dart

@@ -32,7 +32,7 @@ export 'src/streamed_response.dart';
 /// the same server, you should use a single [Client] for all of those requests.
 ///
 /// For more fine-grained control over the request, use [Request] instead.
-Future<Response> head(url, {Map<String, String> headers}) =>
+FutureOr<Response> head(url, {Map<String, String> headers}) =>


Uh...these are always async, right?

Why the move to FutureOr?

@kevmoo the intent is to move to middleware like shelf.

Okay...but do we imagine a future where these calls may be sync?

Are we just talking http.dart or throughout? With the http helpers no.

Exactly. So this method should always return Future. FutureOr implies that it may synchronously return Response.

googlebot · 2017-02-17T17:58:30Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

googlebot · 2017-02-17T18:00:35Z

CLAs look good, thanks!

donny-dont · 2017-02-17T18:15:33Z

@kevmoo fixed the interface.

nex3 · 2017-02-19T20:45:13Z

lib/http.dart

@@ -161,7 +157,7 @@ Future<String> read(url, {Map<String, String> headers}) =>
 Future<Uint8List> readBytes(url, {Map<String, String> headers}) =>
  _withClient((client) => client.readBytes(url, headers: headers));

-Future/*<T>*/ _withClient/*<T>*/(Future/*<T>*/ fn(Client client)) async {
+Future<T> _withClient<T>(FutureOr<T> fn(Client client)) async {


Why is this FutureOr? I'm pretty sure all the functions return Futures.

nex3 · 2017-02-19T20:45:41Z

lib/src/base_client.dart

@@ -23,15 +21,15 @@ abstract class BaseClient implements Client {
  /// can be a [Uri] or a [String].
  ///
  /// For more fine-grained control over the request, use [send] instead.
-  Future<Response> head(url, {Map<String, String> headers}) =>
-    _sendUnstreamed("HEAD", url, headers);
+  FutureOr<Response> head(url, {Map<String, String> headers}) =>


Same question here: why FutureOr? This is never going to return a Response synchronously.

nex3 · 2017-02-19T20:47:35Z

lib/src/base_client.dart

-  Future<Response> head(url, {Map<String, String> headers}) =>
-    _sendUnstreamed("HEAD", url, headers);
+  FutureOr<Response> head(url, {Map<String, String> headers}) =>
+    send(new Request.head(_uri(url), headers: headers));


The request constructors should take dynamic urls, not Uris.

How about the named constructors take dynamic but the default takes a Uri? If all of them take dynamic then there's a check on each call to change.

I'd rather have a cleaner public API and add a private constructor for change to call, I think.

K works for me 👍

donny-dont · 2017-02-23T21:40:53Z

@nex3 @kevmoo think we're ready to go know.

kevmoo · 2017-02-27T18:22:23Z

pubspec.yaml

@@ -4,7 +4,7 @@ author: "Dart Team <misc@dartlang.org>"
 homepage: https://github.com/dart-lang/http


We should make sure to update the pubspec version to make a major change, too.

Updated to 0.12.0

This should be 0.12.0-dev. We use the "-dev" suffix to clearly distinguish between released versions and those that are still in progress.

nex3

Sorry it took me so long to get back to this!

nex3 · 2017-03-01T22:03:37Z

lib/src/request.dart

+  }
+}
+
+Uri _uri(url) {


Either make this a static method of Request, or put it in utils.

nex3 · 2017-03-01T22:06:11Z

lib/src/request.dart

@@ -30,12 +30,12 @@ class Request extends Message {
  ///
  /// Extra [context] can be used to pass information between inner middleware
  /// and handlers.
-  Request(this.method, this.url,
+  Request(String method, url,


Document the acceptable types for url. You can probably just copy the docs from the top-level methods.

nex3 · 2017-03-01T22:07:05Z

pubspec.yaml

@@ -4,7 +4,7 @@ author: "Dart Team <misc@dartlang.org>"
 homepage: https://github.com/dart-lang/http


This should be 0.12.0-dev. We use the "-dev" suffix to clearly distinguish between released versions and those that are still in progress.

donny-dont · 2017-03-06T22:55:59Z

🎊

donny-dont added 5 commits February 16, 2017 22:43

Remove extensions of request and response

d274582

Update client interface

bb81377

Update async to include collectBytes

9764158

Implement updated interface in BaseClient

cf91544

Update helper functions

949bb56

googlebot added the cla: yes label Feb 17, 2017

kevmoo reviewed Feb 17, 2017

View reviewed changes

googlebot added cla: no and removed cla: yes labels Feb 17, 2017

donny-dont force-pushed the feature/client-interface branch from 0ef3052 to 0cd3d44 Compare February 17, 2017 18:00

googlebot added cla: yes and removed cla: no labels Feb 17, 2017

Use Future in http helpers

6cf06ea

donny-dont force-pushed the feature/client-interface branch from 0cd3d44 to 6cf06ea Compare February 17, 2017 18:04

nex3 suggested changes Feb 19, 2017

View reviewed changes

donny-dont added 2 commits February 22, 2017 21:58

Modify Request to take dynamic for urls

6df8771

Remove uses of FutureOr

07dd399

kevmoo requested changes Feb 27, 2017

View reviewed changes

Update version

69df648

kevmoo approved these changes Feb 27, 2017

View reviewed changes

nex3 mentioned this pull request Mar 1, 2017

Please bump version to 1.0 when stable #60

Closed

nex3 suggested changes Mar 1, 2017

View reviewed changes

donny-dont and others added 3 commits March 1, 2017 18:24

Use dev for version

35a765c

Move url to utils and add additional documentation

8d2e503

Fix a typo.

362ad9c

nex3 approved these changes Mar 6, 2017

View reviewed changes

Merge branch 'master' into feature/client-interface

4285911

nex3 merged commit e9dbebd into dart-lang:master Mar 6, 2017

donny-dont deleted the feature/client-interface branch March 6, 2017 22:55

aliaafreen mentioned this pull request Nov 24, 2022

SocketException: Connection timed out (OS Error: Connection timed out, errno = 110) #831

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Client interface #56

Update Client interface #56

donny-dont commented Feb 17, 2017

kevmoo Feb 17, 2017

donny-dont Feb 17, 2017

kevmoo Feb 17, 2017

donny-dont Feb 17, 2017

kevmoo Feb 17, 2017

googlebot commented Feb 17, 2017

googlebot commented Feb 17, 2017

donny-dont commented Feb 17, 2017

nex3 Feb 19, 2017

nex3 Feb 19, 2017

nex3 Feb 19, 2017

donny-dont Feb 19, 2017

nex3 Feb 21, 2017

donny-dont Feb 22, 2017

donny-dont commented Feb 23, 2017

kevmoo Feb 27, 2017

donny-dont Feb 27, 2017

kevmoo Feb 27, 2017

nex3 Mar 1, 2017

nex3 left a comment

nex3 Mar 1, 2017

nex3 Mar 1, 2017

nex3 Mar 1, 2017

donny-dont commented Mar 6, 2017

		@@ -4,7 +4,7 @@ author: "Dart Team <misc@dartlang.org>"
		homepage: https://github.com/dart-lang/http

Update Client interface #56

Update Client interface #56

Conversation

donny-dont commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Feb 17, 2017

googlebot commented Feb 17, 2017

donny-dont commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donny-dont commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nex3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donny-dont commented Mar 6, 2017