Adding middleware #85

donny-dont · 2017-05-25T00:59:31Z

Currently a work in progress to see if this is on the right track. Will add documentation during the review process.

nex3 · 2017-05-25T18:38:29Z

lib/src/client.dart

@@ -28,6 +29,9 @@ abstract class Client {
  /// [BrowserClient] if `dart:html` is available.
  factory Client() => new IOClient();

+  factory Client.handler(Handler handler, {CloseHandler onClose})


Nit: I don't like using function typedefs in parameters, because it's much harder to quickly figure out what they are. Handler is okay since it's such a fundamental type, but I'd write void onClose().

nex3 · 2017-05-25T18:38:49Z

lib/src/handler_client.dart

+import 'response.dart';
+
+typedef Future<Response> Handler(Request request);
+typedef void CloseHandler();


Similarly, I wouldn't make this a public type.

Fixed. Would be totally rid of it but no types on the right 😞

Actually if I can target 1.24 then we can get rid of the typedef since there's now a syntax for it. Let me know if its ok to do that.

Yeah, that sounds fine—1.24 will probably be out before this is ready for release.

nex3 · 2017-05-25T18:40:47Z

lib/src/middleware.dart

+        closeHandler();
+
+        inner.close();
+      },


It's a small thing, but it's probably more efficient to directly pass in inner.close if closeHandler() isn't passed.

It would be but this ensures that close goes down through the pipeline.

As an aside into performance would it be better to just have

Future<Request> _defaultRequest(Request request) async => request; Future<Response> _defaultResponse(Response response) async => response;

etc

It would be but this ensures that close goes down through the pipeline.

I'm not sure what you mean by "goes down the pipeline".

As an aside into performance would it be better to just have

Future<Request> _defaultRequest(Request request) async => request; Future<Response> _defaultResponse(Response response) async => response;

etc

Nah, it would be trivial for implementations to hoist these if it's actually relevant to performance.

onClose is a void () function. Within the middleware it will be unable to call inner.close because the Client is not passed to it. Also this makes things safer because the inner.close is always called and someone writing a middleware can't forget to call close.

I'm suggesting you write:

onClose: closeHandler == null ? inner.close : () { closeHandler(); inner.close(); }

nex3 · 2017-05-25T18:41:45Z

lib/src/middleware.dart

+Middleware createMiddleware({
+  RequestHandler requestHandler,
+  ResponseHandler responseHandler,
+  CloseHandler closeHandler,


I'd call this onClose, for consistency with new Client.handler() and other Dart APIs. It's also not really a handler, since it's not invoked for a request.

Do you want this to be

onRequest, onResponse, onClose?

No, I think handle is good for the first two.

nex3 · 2017-05-25T18:42:23Z

lib/src/middleware.dart

+
+Middleware createMiddleware({
+  RequestHandler requestHandler,
+  ResponseHandler responseHandler,


It's probably a good idea to add an errorHandler here that works the same as shelf's.

Do you want the same HijackException functionality?

Not right now. We may find a compelling use-case at some point, but I think it's a lot less crucial on the client since you aren't trying to mount a WebSocket handler as part of an existing app or whatever.

nex3 · 2017-05-25T18:42:50Z

lib/src/middleware.dart

+  responseHandler ??= (response) async => response;
+  closeHandler ??= () {};
+
+  return (Client inner) {


Don't type anonymous function parameters.

nex3 · 2017-05-25T18:42:58Z

lib/src/middleware.dart

+          (Request request) async {
+        request = await requestHandler(request);
+
+        final response = await inner.send(request);


nex3 · 2017-05-26T23:52:13Z

lib/src/handler_client.dart

+import 'request.dart';
+import 'response.dart';
+
+typedef Future<Response> Handler(Request request);


I'd put this in its own library. I know it seems silly when it's just one line of code, but having one file per type makes them a lot easier to track down in the source.

nex3 · 2017-05-26T23:54:16Z

lib/src/middleware.dart

+
+Middleware createMiddleware({
+  RequestHandler requestHandler,
+  ResponseHandler responseHandler,


Not right now. We may find a compelling use-case at some point, but I think it's a lot less crucial on the client since you aren't trying to mount a WebSocket handler as part of an existing app or whatever.

nex3 · 2017-05-26T23:54:31Z

lib/src/middleware.dart

+Middleware createMiddleware({
+  RequestHandler requestHandler,
+  ResponseHandler responseHandler,
+  CloseHandler closeHandler,


No, I think handle is good for the first two.

nex3 · 2017-05-26T23:56:13Z

lib/src/middleware.dart

+        closeHandler();
+
+        inner.close();
+      },


It would be but this ensures that close goes down through the pipeline.

I'm not sure what you mean by "goes down the pipeline".

As an aside into performance would it be better to just have

Future<Request> _defaultRequest(Request request) async => request; Future<Response> _defaultResponse(Response response) async => response;

etc

Nah, it would be trivial for implementations to hoist these if it's actually relevant to performance.

donny-dont · 2017-05-27T00:32:55Z

@nex3 let me know if everything other than the error handler bits are looking good. If so I can start adding some documentation to it all as well.

nex3

Everything other than the error handler looks good so far.

nex3 · 2017-05-30T20:02:30Z

lib/src/handler_client.dart

+import 'response.dart';
+
+typedef Future<Response> Handler(Request request);
+typedef void CloseHandler();


Yeah, that sounds fine—1.24 will probably be out before this is ready for release.

nex3 · 2017-05-30T20:02:42Z

lib/src/middleware.dart

+        closeHandler();
+
+        inner.close();
+      },


I'm suggesting you write:

onClose: closeHandler == null ? inner.close : () { closeHandler(); inner.close(); }

donny-dont · 2017-05-30T20:08:50Z

@nex3 I added some error handling any particular issues with it? It follows along with what I saw in shelf

nex3

Error handling looks good.

I'm wondering if the onClose parameter to new HandlerClient should be optional. I can imagine handlers that don't want to do anything when they're closed—forwarding directly to an in-process shelf handler, for example.

donny-dont · 2017-05-31T22:54:30Z

@nex3 HandlerClient isn't exposed. Its only available through the factory constructor in Client. If you would rather expose it directly then we can do that.

nex3 · 2017-05-31T22:56:34Z

Oh, I forgot. In that case, it's fine as-is.

donny-dont · 2017-05-31T22:57:40Z

I'll go ahead and add some documentation then this should be good to go 👍

donny-dont · 2017-06-12T20:48:35Z

@nex3 docs look alright?

nex3 · 2017-06-12T22:21:03Z

lib/src/client.dart

@@ -30,6 +30,13 @@ abstract class Client {
  /// [BrowserClient] if `dart:html` is available.
  factory Client() => new IOClient();

+  /// Creates a new client handler.


Maybe "Creates a new Client from a [handler] callback." The client itself isn't a handler in the type-system sense, so we should avoid implying that in the docs.

nex3 · 2017-06-12T22:21:48Z

lib/src/client.dart

+  ///
+  /// The [handler] is a function that receives a [Request] and returns a
+  /// [Future<Response>]. It will be called when [Client.send] is invoked. This
+  /// allows for composition of a [Client] within a larger application.


This last sentence is confusing. It's not clear to me why this constructor in particular makes composition easier.

nex3 · 2017-06-12T22:23:35Z

lib/src/handler.dart

+/// [Future<Response>].
+///
+/// A [Handler] may receive a request directly for a HTTP resource or it may be
+/// composed as part of a larger application.


It's not clear what it means to receive a request "directly". Maybe, "A [Handler] may call an underlying HTTP implementation, or it may wrap another [Handler] or a [Client]."

nex3 · 2017-06-12T22:23:59Z

lib/src/handler_client.dart

@@ -9,15 +9,26 @@ import 'handler.dart';
 import 'request.dart';
 import 'response.dart';

+/// A [Handler] based HTTP client.


nit: "[Handler] based" -> "[Handler]-based"

nex3 · 2017-06-12T22:24:30Z

lib/src/handler_client.dart

  HandlerClient(this._handler, void onClose())
      : _close = onClose;

+  /// Sends an HTTP request and asynchronously returns the response.
+  ///
+  /// The [Handler] function will be called when this method is invoked.


I don't think this line is necessary, especially since you can't directly refer to the values. Same for the similar line in close().

nex3 · 2017-06-22T20:55:29Z

Thanks!!

Adding middleware

76b7c91

googlebot added the cla: yes label May 25, 2017

nex3 reviewed May 25, 2017

View reviewed changes

donny-dont added 2 commits May 25, 2017 14:32

Fixing some nits

b3a26cf

Fixing another nit

c693884

nex3 suggested changes May 26, 2017

View reviewed changes

More review fixes

afcde17

donny-dont added 2 commits May 26, 2017 22:19

Add error handler

b7c8351

Use futures to support error handling

ec184b0

nex3 approved these changes May 30, 2017

View reviewed changes

Fixing more feedback

d9a281c

nex3 approved these changes May 31, 2017

View reviewed changes

Add documentation

4260f9f

nex3 suggested changes Jun 12, 2017

View reviewed changes

This was referenced Jun 21, 2017

Would like to cache HTTP responses on disk flutter/flutter#10820

Closed

Add Pipeline implementation #87

Merged

Add Cascade implementation #88

Closed

Fix documentation

5382a36

nex3 approved these changes Jun 22, 2017

View reviewed changes

nex3 merged commit 5466bac into dart-lang:master Jun 22, 2017

aliaafreen mentioned this pull request Nov 24, 2022

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

Closed

Adding middleware #85

Adding middleware #85

Conversation

donny-dont commented May 25, 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

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

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

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

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 May 27, 2017

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

donny-dont commented May 30, 2017

nex3 left a comment

Choose a reason for hiding this comment

donny-dont commented May 31, 2017

nex3 commented May 31, 2017

donny-dont commented May 31, 2017

donny-dont commented Jun 12, 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

nex3 commented Jun 22, 2017