Add a Server interface.

Add a Server interface.

A Server represents an adapter that knows its own URL. It's a useful
abstraction for code that needs to know its location in URL-space but
doesn't want to tightly couple itself to a given server implementation.

R=kevmoo@google.com

Committed: https://github.com/dart-lang/shelf/commit/75b201c03415a0aba0f8b7f9add582e8a3d36c57

[kevmoo, 2015-10-22 00:44:26]

I'd love a bit more explanation here. Maybe update the sample?

I'm not clear how/where this would be useful. Should shelf_appengine be updated
to use this? How? Why?

https://codereview.chromium.org/1411553006/diff/1/lib/src/io_server.dart
File lib/src/io_server.dart (right):

https://codereview.chromium.org/1411553006/diff/1/lib/src/io_server.dart#newc...
lib/src/io_server.dart:55: serveRequests(server, handler);
_mounted is never set

You get a state error for multiple subscriptions, so the tests pass – but we
should make sure folks get the helpful error above

https://codereview.chromium.org/1411553006/diff/1/lib/src/server_handler.dart
File lib/src/server_handler.dart (right):

https://codereview.chromium.org/1411553006/diff/1/lib/src/server_handler.dart...
lib/src/server_handler.dart:55: return _server._onMounted.then((_) =>
_server._handler(request));
Why don't you use awa...oh.

https://codereview.chromium.org/1411553006/diff/1/test/io_server_test.dart
File test/io_server_test.dart (right):

https://codereview.chromium.org/1411553006/diff/1/test/io_server_test.dart#ne...
test/io_server_test.dart:11: import 'package:shelf/shelf.dart';
unused import

https://codereview.chromium.org/1411553006/diff/1/test/io_server_test.dart#ne...
test/io_server_test.dart:18: var server;
type things when there is no way for inference to work

The analyzer can help us find bugs, etc

https://codereview.chromium.org/1411553006/diff/1/test/io_server_test.dart#ne...
test/io_server_test.dart:39: expect(() => server.mount((_) {}),
throwsStateError);
actually check for your StateError message – since the stream subscription error
is also a StateError, but with a much less helpful message

[nweiz, 2015-10-26 21:32:44]

> I'd love a bit more explanation here. Maybe update the
> sample?
> 
> I'm not clear how/where this would be useful.

I feel like the Server class has pretty thorough documentation. Can you help me
understand what it doesn't cover?

> Should shelf_appengine be updated to use this? How? Why?

Yes. As the README says, "an adapter that knows its own URL should provide an
implementation of the Server interface." The how and why are covered to the best
of my ability by the Server documentation.

https://codereview.chromium.org/1411553006/diff/1/lib/src/io_server.dart
File lib/src/io_server.dart (right):

https://codereview.chromium.org/1411553006/diff/1/lib/src/io_server.dart#newc...
lib/src/io_server.dart:55: serveRequests(server, handler);
On 2015/10/22 00:44:25, kevmoo wrote:
> _mounted is never set
> 
> You get a state error for multiple subscriptions, so the tests pass – but
we
> should make sure folks get the helpful error above

Done.

https://codereview.chromium.org/1411553006/diff/1/test/io_server_test.dart
File test/io_server_test.dart (right):

https://codereview.chromium.org/1411553006/diff/1/test/io_server_test.dart#ne...
test/io_server_test.dart:11: import 'package:shelf/shelf.dart';
On 2015/10/22 00:44:26, kevmoo wrote:
> unused import

Done.

https://codereview.chromium.org/1411553006/diff/1/test/io_server_test.dart#ne...
test/io_server_test.dart:18: var server;
On 2015/10/22 00:44:26, kevmoo wrote:
> type things when there is no way for inference to work
> 
> The analyzer can help us find bugs, etc

This is a test—if there are bugs the tests will fail, and it's clear to the user
what the type of this is. Also, typing it would be inconsistent with a huge
swath of other similar test code out.

https://codereview.chromium.org/1411553006/diff/1/test/io_server_test.dart#ne...
test/io_server_test.dart:39: expect(() => server.mount((_) {}),
throwsStateError);
On 2015/10/22 00:44:26, kevmoo wrote:
> actually check for your StateError message – since the stream
subscription error
> is also a StateError, but with a much less helpful message

Checking for specific messages in tests is generally an anti-pattern; it makes
the tests much more fragile and adds very little value. We shouldn't have to
change a bunch of tests if we change an error message.

[nweiz, 2015-10-26 22:16:36]

Code review changes

[kevmoo, 2015-10-26 22:17:36]

lgtm

[nweiz, 2015-10-26 23:02:08]

Committed patchset #2 (id:20001) manually as
75b201c03415a0aba0f8b7f9add582e8a3d36c57 (presubmit successful).