Support serving from multiple directories using "pub serve".

Support serving from multiple directories using "pub serve".

R=rnystrom@google.com
BUG=15924
Committed: https://code.google.com/p/dart/source/detail?r=32818

[Bob Nystrom, 2014-02-19 00:36:52]

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/build_environment.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:40: /// necessary.
If [basePort] is zero, each server will have an arbitrarily-allocated port.
Long line.

Also, "arbitrarily-allocated" -> "ephemeral".

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:101: // the
following ports in alphabetical order.
Since create() takes an iterable and not a set and the serve command defines
this policy, how about we just rely on serve passing in "web" and "test" as the
first two elements in the iterable instead of redundantly enforcing the policy
here too?

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:110: var
remainingDirectories = ordered(rootDirectories);
I think it makes more sense to just preserve the order the user passed them in.
That may make it easier for users to script pub serve.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:190:
servers.first.results.listen((_) {}, onError: (error, stackTrace) {
What about the others?

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:370:
environment.servers.first.port);
Document why using .first is OK.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:88:
'${commands[name].description.split("\n").first}');
How about some tests that validate when the description is truncated and when it
isn't?

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/serve.dart:112:
padRight("[${server.rootDirectory}/]", directoryLength + 3));
Nit, but I think I'd prefer not showing the "/" here. I wonder if it will just
be confusing to Windows users.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/serve.dart:143: /// the user.
Document that it may throw a UsageException if the command line arguments are
invalid.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/log.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:241: /// Wraps [text] in the ANSI escape
codes to make it bold when on a platform
"bold" -> "gray".

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/roots/serves_urls_from_custom_roots_test.dart
(right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/serves_urls_from_custom_roots_test.dart:27:
requestShouldSucceed("baz", "contents", root: "dir");
Test that this 404s on something that does exist in web and test.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/roots/serves_web_and_test_dirs_by_default_test.dart
(right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/serves_web_and_test_dirs_by_default_test.dart:22:
requestShouldSucceed("bar", "contents", root: "test");
Test that it 404s on something in example.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_by_default_if_web_and_test_dont_exist_test.dart
(right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_by_default_if_web_and_test_dont_exist_test.dart:24:
server.stderr.expect('Neither "web/" nor "test/" exist to serve from.');
How about:

"Your package must have a "web" or "test" directory to serve."

?

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_if_custom_roots_dont_exist_test.dart
(right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_if_custom_roots_dont_exist_test.dart:17:
integration("throws an error if custom roots don't exist", () {
Is this the behavior we want? What if the user fires up pub serve and then
creates the directory afterwards?

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_if_custom_roots_dont_exist_test.dart:22:
var server = startPubServe(args: ["foo", "bar"]);
Add a third directory that does exist to ensure that we only show errors for the
missing ones, and that we do show errors even if some are missing.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_if_custom_roots_dont_exist_test.dart:23:
server.stderr.expect('Directories "bar/" and "foo/" don\'t exist.');
Remove the "/" from the names. We don't use them when referring to directories
elsewhere, and it will be wrong on Windows.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:148:
_pubServer.stdout.expect(predicate(_parsePort, 'emits server url'));
Given the isNot(isEmpty) below, is this line necessary?

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:249: var port = _ports.values.first;
Do we intend to support the Web Socket API on every port? Just the first? A
separate one?

I think once we have a separate port for a web interface, we should probably
just support the Web Socket API on that one. Add a TODO to that effect?

[nweiz, 2014-02-19 01:25:57]

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/build_environment.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:40: /// necessary.
If [basePort] is zero, each server will have an arbitrarily-allocated port.
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Long line.
> 
> Also, "arbitrarily-allocated" -> "ephemeral".

Done.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:101: // the
following ports in alphabetical order.
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Since create() takes an iterable and not a set and the serve command defines
> this policy, how about we just rely on serve passing in "web" and "test" as
the
> first two elements in the iterable instead of redundantly enforcing the policy
> here too?

The issue with that is that port 8080 should always mean "web" and 8081 should
always mean test; if the user writes "pub serve example", example should be on
8082, not 8080.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:110: var
remainingDirectories = ordered(rootDirectories);
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> I think it makes more sense to just preserve the order the user passed them
in.
> That may make it easier for users to script pub serve.

Done.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:190:
servers.first.results.listen((_) {}, onError: (error, stackTrace) {
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> What about the others?

This just catches errors during the transformer initialization, which only uses
the first server. I'll add a comment to that effect.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/load_transformers.dart:370:
environment.servers.first.port);
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Document why using .first is OK.

Done.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command.dart:88:
'${commands[name].description.split("\n").first}');
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> How about some tests that validate when the description is truncated and when
it
> isn't?

Done.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/command/serve.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/serve.dart:112:
padRight("[${server.rootDirectory}/]", directoryLength + 3));
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Nit, but I think I'd prefer not showing the "/" here. I wonder if it will just
> be confusing to Windows users.

Done.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/command/serve.dart:143: /// the user.
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Document that it may throw a UsageException if the command line arguments are
> invalid.

Done.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/log.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/log.dart:241: /// Wraps [text] in the ANSI escape
codes to make it bold when on a platform
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> "bold" -> "gray".

Done.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/roots/serves_urls_from_custom_roots_test.dart
(right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/serves_urls_from_custom_roots_test.dart:27:
requestShouldSucceed("baz", "contents", root: "dir");
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Test that this 404s on something that does exist in web and test.

Done.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/roots/serves_web_and_test_dirs_by_default_test.dart
(right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/serves_web_and_test_dirs_by_default_test.dart:22:
requestShouldSucceed("bar", "contents", root: "test");
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Test that it 404s on something in example.

Done.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_by_default_if_web_and_test_dont_exist_test.dart
(right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_by_default_if_web_and_test_dont_exist_test.dart:24:
server.stderr.expect('Neither "web/" nor "test/" exist to serve from.');
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> How about:
> 
> "Your package must have a "web" or "test" directory to serve."
> 
> ?

That sounds like you're saying "in order to use 'pub serve' you have to have
'web' or 'test'", which isn't accurate since you can pass other directories.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_if_custom_roots_dont_exist_test.dart
(right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_if_custom_roots_dont_exist_test.dart:17:
integration("throws an error if custom roots don't exist", () {
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Is this the behavior we want? What if the user fires up pub serve and then
> creates the directory afterwards?

The watcher package doesn't support that. We currently allow this for "pub
serve", but if you create the directory after the fact no changes in it will
ever be registered.

We could theoretically support this by watching the parent directory to see when
the child directories were created, then watching those, but that sounds like
more pain than it's worth.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_if_custom_roots_dont_exist_test.dart:22:
var server = startPubServe(args: ["foo", "bar"]);
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Add a third directory that does exist to ensure that we only show errors for
the
> missing ones, and that we do show errors even if some are missing.

Done.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_if_custom_roots_dont_exist_test.dart:23:
server.stderr.expect('Directories "bar/" and "foo/" don\'t exist.');
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Remove the "/" from the names. We don't use them when referring to directories
> elsewhere, and it will be wrong on Windows.

Done.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File sdk/lib/_internal/pub/test/serve/utils.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:148:
_pubServer.stdout.expect(predicate(_parsePort, 'emits server url'));
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Given the isNot(isEmpty) below, is this line necessary?

Nope, removed.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/utils.dart:249: var port = _ports.values.first;
On 2014/02/19 00:36:52, Bob Nystrom wrote:
> Do we intend to support the Web Socket API on every port? Just the first? A
> separate one?
> 
> I think once we have a separate port for a web interface, we should probably
> just support the Web Socket API on that one. Add a TODO to that effect?

Done.

[Bob Nystrom, 2014-02-19 01:50:54]

LGTM.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
File sdk/lib/_internal/pub/lib/src/barback/build_environment.dart (right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/lib/sr...
sdk/lib/_internal/pub/lib/src/barback/build_environment.dart:101: // the
following ports in alphabetical order.
On 2014/02/19 01:25:58, nweiz wrote:
> On 2014/02/19 00:36:52, Bob Nystrom wrote:
> > Since create() takes an iterable and not a set and the serve command defines
> > this policy, how about we just rely on serve passing in "web" and "test" as
> the
> > first two elements in the iterable instead of redundantly enforcing the
policy
> > here too?
> 
> The issue with that is that port 8080 should always mean "web" and 8081 should
> always mean test; if the user writes "pub serve example", example should be on
> 8082, not 8080.

I personally don't care that much about always guaranteeing the ports for web
and test, but if it's important to you, OK.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_by_default_if_web_and_test_dont_exist_test.dart
(right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_by_default_if_web_and_test_dont_exist_test.dart:24:
server.stderr.expect('Neither "web/" nor "test/" exist to serve from.');
On 2014/02/19 01:25:58, nweiz wrote:
> On 2014/02/19 00:36:52, Bob Nystrom wrote:
> > How about:
> > 
> > "Your package must have a "web" or "test" directory to serve."
> > 
> > ?
> 
> That sounds like you're saying "in order to use 'pub serve' you have to have
> 'web' or 'test'", which isn't accurate since you can pass other directories.

Maybe, "Your package must have a "web" or "test" directory to serve, or you must
pass in the directories to serve explicitly."

?

That's not great either, but something along those lines.

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_if_custom_roots_dont_exist_test.dart
(right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_if_custom_roots_dont_exist_test.dart:17:
integration("throws an error if custom roots don't exist", () {
On 2014/02/19 01:25:58, nweiz wrote:
> On 2014/02/19 00:36:52, Bob Nystrom wrote:
> > Is this the behavior we want? What if the user fires up pub serve and then
> > creates the directory afterwards?
> 
> The watcher package doesn't support that. We currently allow this for "pub
> serve", but if you create the directory after the fact no changes in it will
> ever be registered.
> 
> We could theoretically support this by watching the parent directory to see
when
> the child directories were created, then watching those, but that sounds like
> more pain than it's worth.

Ah good point. Let's not worry about that then.

[nweiz, 2014-02-19 20:39:17]

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
File
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_by_default_if_web_and_test_dont_exist_test.dart
(right):

https://codereview.chromium.org/167103003/diff/1/sdk/lib/_internal/pub/test/s...
sdk/lib/_internal/pub/test/serve/roots/throws_an_error_by_default_if_web_and_test_dont_exist_test.dart:24:
server.stderr.expect('Neither "web/" nor "test/" exist to serve from.');
On 2014/02/19 01:50:55, Bob Nystrom wrote:
> On 2014/02/19 01:25:58, nweiz wrote:
> > On 2014/02/19 00:36:52, Bob Nystrom wrote:
> > > How about:
> > > 
> > > "Your package must have a "web" or "test" directory to serve."
> > > 
> > > ?
> > 
> > That sounds like you're saying "in order to use 'pub serve' you have to have
> > 'web' or 'test'", which isn't accurate since you can pass other directories.
> 
> Maybe, "Your package must have a "web" or "test" directory to serve, or you
must
> pass in the directories to serve explicitly."
> 
> ?
> 
> That's not great either, but something along those lines.

Done.

[nweiz, 2014-02-19 20:39:48]

Committed patchset #3 manually as r32818 (presubmit successful).