Emulate shelf messaging #54

donny-dont · 2017-01-14T17:22:07Z

Aligns http's Request and Response with what is present in shelf

donny-dont · 2017-01-14T17:23:14Z

In headers.dart only the getHeader is currently being used. It was present in shelf so its there.

nex3

This is looking good! I have a fair number of comments, but they're all fairly minor.

nex3 · 2017-01-17T21:37:24Z

lib/src/body.dart

@@ -44,14 +42,14 @@ class Body {
      stream = new Stream.fromIterable([]);
    } else if (body is String) {
      if (encoding == null) {
-        var encoded = UTF8.encode(body);
+        final encoded = UTF8.encode(body);


final locals is emphatically not the style this package (or pretty much any Dart-team-authored code) uses.

K I can walk those changes back.

nex3 · 2017-01-17T21:41:34Z

lib/src/headers.dart

+/// Adds a header with [name] and [value] to [headers], which may be null.
+///
+/// Returns a new map without modifying [headers].
+Map<String, String> addHeader(


Is this actually used? In shelf, it's only used for adding headers for specialized Response constructors that don't exist here.

Just getHeader is used so far. I wasn't completely positive if the others would be of use as we moved further. I'll remove and add back if needed.

nex3 · 2017-01-17T21:41:58Z

lib/src/headers.dart

+///
+/// This works even if [headers] is `null`, or if it's not yet a
+/// case-insensitive map.
+bool hasHeader(Map<String, String> headers, String name) {


This also looks like it's not used.

nex3 · 2017-01-17T21:44:04Z

lib/src/http_unmodifiable_map.dart

  const _EmptyHttpUnmodifiableMap() : super(const {});
+
+  bool get _ignoreKeyCase => true;


Why this change?

There's a lint rule there that I had enabled, http://dart-lang.github.io/linter/lints/sort_constructors_first.html

I can change it back if you desire. I like the rule because it keeps things consistent.

Yes. In general, please don't change things here just because lints say to—this package doesn't use the linter and it causes churn.

nex3 · 2017-01-17T21:45:09Z

lib/src/message.dart

+import 'headers.dart';
+import 'http_unmodifiable_map.dart';
+
+Body getBody(Message message) => message._body;


Document this.

Can do. It wasn't documented in shelf so I had assumed it was more of a utility function.

nex3 · 2017-01-17T22:01:23Z

lib/src/request.dart

+      Map<String, Object> context})
+      : super(body, encoding: encoding, headers: headers, context: context);
+
+  /// Creates a new [Request] for [url] using a "HEAD" method.


I'd phrase these as e.g. Creates a new HEAD [Request] to [url].

nex3 · 2017-01-17T22:03:54Z

lib/src/request.dart

+      : this('GET', url, headers: headers, context: context);
+
+  /// Creates a new [Request] for [url] using a "POST" method.
+  Request.post(Uri url,


Constructors for methods that typically have entities should make the body a required parameter.

nex3 · 2017-01-17T22:14:20Z

lib/src/response.dart

-  /// using the `charset` parameter of the `Content-Type` header field, if
-  /// available. If it's unavailable or if the encoding name is unknown,
-  /// [LATIN1] is used by default, as per [RFC 2616][].
+class Response extends Message {


At some point this should utility getters like shelf.Response.expires et al. That doesn't need to happen in this PR though.

nex3 · 2017-01-17T22:15:08Z

lib/src/request.dart

+      Map<String, Object> context,
+      dynamic body}) {
+    final updatedHeaders = updateMap/*<String, String>*/(this.headers, headers);
+    final updatedContext = updateMap/*<String, Object>*/(this.context, context);


Are these explicit type annotations necessary? Can't it infer the type from the parameters?

nex3 · 2017-01-17T22:15:23Z

lib/src/utils.dart

+  map.forEach((key, value) => pairs.add([
+        Uri.encodeQueryComponent(key, encoding: encoding),
+        Uri.encodeQueryComponent(value, encoding: encoding)
+      ]));


Please don't include unrelated formatting changes.

donny-dont · 2017-01-19T04:18:09Z

Okay @nex3 I think I have everything resolved.

nex3 · 2017-01-24T23:43:37Z

lib/src/http_unmodifiable_map.dart

  const _EmptyHttpUnmodifiableMap() : super(const {});
+
+  bool get _ignoreKeyCase => true;


Yes. In general, please don't change things here just because lints say to—this package doesn't use the linter and it causes churn.

nex3 · 2017-01-24T23:46:48Z

lib/src/request.dart

+  /// The URL to which the request will be sent.
+  final Uri url;
+
+  /// Creates a new [Request] for [url] using [method].


nex3 · 2017-01-24T23:52:38Z

lib/src/message.dart

@@ -12,11 +12,12 @@ import 'body.dart';
 import 'headers.dart';
 import 'http_unmodifiable_map.dart';

+/// Retrieves the [Body] contained in the [message].


Explain that it's a separate function so the message body is accessible within shelf but hidden elsewhere.

nex3 · 2017-01-24T23:57:01Z

lib/src/message.dart

 Body getBody(Message message) => message._body;

 /// The default set of headers for a message created with no body and no
 /// explicit headers.
-final _defaultHeaders = new HttpUnmodifiableMap<String>({"content-length": "0"},
+final _defaultHeaders = new HttpUnmodifiableMap<String>({},


I don't think just making this empty is enough. We still need to send content-length: 0 for null-bodied POSTs and other requests that have content.

Ok so I'm not totally sure the best way to get all this working as you would expect. So how about I create unit tests on what your expectation is and we go from there?

Actually, re-reading the spec, it says

The presence of a message-body in a request is signaled by the inclusion of a Content-Length or Transfer-Encoding header field in the request's message-headers.

So it's probably fine to just not include a content-length header for requests that have no entity body, regardless of their method. We'll need to make sure that dart:io doesn't force a chunked transfer encoding in those cases, though.

nex3 · 2017-01-24T23:57:56Z

lib/src/message.dart

  ///
  /// For requests, this is used to pass data to inner middleware and handlers;
  /// for responses, it's used to pass data to outer middleware and handlers.
  ///
  /// Context properties that are used by a particular package should begin with
-  /// that package's name followed by a period. For example, if [logRequests]
+  /// that package's name followed by a period. For example, if `logRequests`


Referring to logRequests here seems strange since that's not middleware that exists—and probably not something this package will ever provide. Maybe rephrase this as describing a hypothetical middleware?

nex3 · 2017-01-24T23:58:13Z

lib/src/message.dart

  ///
  /// For requests, this is used to pass data to inner middleware and handlers;
  /// for responses, it's used to pass data to outer middleware and handlers.
  ///
  /// Context properties that are used by a particular package should begin with
-  /// that package's name followed by a period. For example, if [logRequests]
+  /// that package's name followed by a period. For example, if `logRequests`
  /// wanted to take a prefix, its property name would be `"http.prefix"`,


"take a prefix" -> "take a context property"

nex3 · 2017-01-24T23:58:57Z

lib/src/request.dart

@@ -7,62 +7,124 @@ import 'dart:convert';
 import 'message.dart';
 import 'utils.dart';

-/// Represents an HTTP request to be processed by a `http` application.
+/// Represents a HTTP request to be sent to a server.


"an" is correct here.

donny-dont added 6 commits January 14, 2017 09:17

Fix linter warnings

e95116e

Add header library from shelf

83b3850

Add message base

b81e654

Add map utility and format

383d755

Add request implementation

ee610aa

Add response implementation

da5819c

googlebot added the cla: yes label Jan 14, 2017

Remove @override annotation

971d1ed

nex3 suggested changes Jan 17, 2017

View reviewed changes

donny-dont added 8 commits January 18, 2017 18:39

Remove unused functions from headers library

1c4c42e

Do not include content length if length is 0

e5c60f8

Revert dartfmt changes

e3b2b1a

Revert dartfmt changes

1b075d9

Adding expires and lastModified to Response

015b0a3

Use var instead of final within function bodies

fb80dbb

Make constructors and change consistent with messages

c047875

Clean up documentation comments

69298a3

nex3 suggested changes Jan 25, 2017

View reviewed changes

donny-dont and others added 3 commits February 12, 2017 17:05

Fixing documentation

2e61b73

Rolling back linter changes

41c5e61

Style changes.

1a54d3b

nex3 approved these changes Feb 14, 2017

View reviewed changes

nex3 merged commit 3ae6d4b into dart-lang:master Feb 14, 2017

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

Emulate shelf messaging #54

Emulate shelf messaging #54

donny-dont commented Jan 14, 2017

donny-dont commented Jan 14, 2017

nex3 left a comment

nex3 Jan 17, 2017

donny-dont Jan 17, 2017

nex3 Jan 17, 2017

donny-dont Jan 17, 2017

nex3 Jan 17, 2017

nex3 Jan 17, 2017

donny-dont Jan 17, 2017

donny-dont Jan 19, 2017

nex3 Jan 24, 2017

nex3 Jan 17, 2017

donny-dont Jan 17, 2017

nex3 Jan 17, 2017

nex3 Jan 17, 2017

donny-dont Jan 19, 2017

nex3 Jan 17, 2017

nex3 Jan 17, 2017

nex3 Jan 17, 2017

donny-dont commented Jan 19, 2017

nex3 Jan 24, 2017

nex3 Jan 24, 2017

nex3 Jan 24, 2017

nex3 Jan 24, 2017

donny-dont Feb 5, 2017

nex3 Feb 6, 2017

nex3 Jan 24, 2017

nex3 Jan 24, 2017

nex3 Jan 24, 2017

		const _EmptyHttpUnmodifiableMap() : super(const {});

		bool get _ignoreKeyCase => true;

Emulate shelf messaging #54

Emulate shelf messaging #54

Conversation

donny-dont commented Jan 14, 2017

donny-dont commented Jan 14, 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

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 Jan 19, 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