Add file watching to pub serve.

Add file watching to pub serve.

R=nweiz@google.com

Committed: https://code.google.com/p/dart/source/detail?r=26242

[nweiz, 2013-08-13 00:48:45]

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

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:89:
watcher.events.listen((event) {
If we're watching for any changes in the entire package, won't this start adding
things outside of lib/, asset/, and web/ to barback?

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:92: log.message("${event.type}
$id");
I'm not a huge fan of exposing the weird "pkg|path" syntax to users. I'd rather
this be the physical path (for assets in the app package) or the
"package:"/"asset:" URI for foreign assets.

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/watch_added_file_test.dart (right):

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/watch_added_file_test.dart:36:
pumpEventQueue();
Pumping the event queue here is weird. Isn't all the relevant processing going
on in the subprocess? What events are happening in the test process that we need
to pump?

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/watch_modified_file_test.dart (right):

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/watch_modified_file_test.dart:32:
requestShouldSucceed("index.html", "after");
If you don't use [waitForOutput], isn't there a race condition here? Couldn't
the request come in and be served before the watcher realizes that index.html
has changed?

[Bob Nystrom, 2013-08-14 23:58:36]

Thanks!

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

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:89:
watcher.events.listen((event) {
On 2013/08/13 00:48:45, nweiz wrote:
> If we're watching for any changes in the entire package, won't this start
adding
> things outside of lib/, asset/, and web/ to barback?

Done.

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/lib/src...
sdk/lib/_internal/pub/lib/src/command/serve.dart:92: log.message("${event.type}
$id");
On 2013/08/13 00:48:45, nweiz wrote:
> I'm not a huge fan of exposing the weird "pkg|path" syntax to users. I'd
rather
> this be the physical path (for assets in the app package) or the
> "package:"/"asset:" URI for foreign assets.

Removed this. I was only using it as a kind of hack so that the tests could look
for that in the output. I have a cleaner way of writing those tests.

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/watch_added_file_test.dart (right):

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/watch_added_file_test.dart:36:
pumpEventQueue();
On 2013/08/13 00:48:45, nweiz wrote:
> Pumping the event queue here is weird. Isn't all the relevant processing going
> on in the subprocess? What events are happening in the test process that we
need
> to pump?

waitForOutput() wasn't actually blocking the schedule (!) and this accidentally
worked around that. Fixed.

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/test/se...
File sdk/lib/_internal/pub/test/serve/watch_modified_file_test.dart (right):

https://codereview.chromium.org/22986002/diff/1/sdk/lib/_internal/pub/test/se...
sdk/lib/_internal/pub/test/serve/watch_modified_file_test.dart:32:
requestShouldSucceed("index.html", "after");
On 2013/08/13 00:48:45, nweiz wrote:
> If you don't use [waitForOutput], isn't there a race condition here? Couldn't
> the request come in and be served before the watcher realizes that index.html
> has changed?

Yup. Fixed!

This test actually would pass even with no watchers hooked up since it yields a
FileAsset for the source, so it always reads the actual file from disc every
time you read it. So the modified contents will go through barback without it
knowing.

[nweiz, 2013-08-15 20:43:30]

One suggestion, otherwise lgtm.

https://codereview.chromium.org/22986002/diff/8001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/pub_package_provider.dart (right):

https://codereview.chromium.org/22986002/diff/8001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/pub_package_provider.dart:57: void
watchSources(Barback barback) {
It feels weird to me to pass a Barback instance in here. I'd either put this and
[_listAssets] back in serve.dart, or create a new file distinct from both the
provider and the command that encapsulates.

[Bob Nystrom, 2013-08-15 23:48:18]

Thanks!

https://codereview.chromium.org/22986002/diff/8001/sdk/lib/_internal/pub/lib/...
File sdk/lib/_internal/pub/lib/src/pub_package_provider.dart (right):

https://codereview.chromium.org/22986002/diff/8001/sdk/lib/_internal/pub/lib/...
sdk/lib/_internal/pub/lib/src/pub_package_provider.dart:57: void
watchSources(Barback barback) {
On 2013/08/15 20:43:30, nweiz wrote:
> It feels weird to me to pass a Barback instance in here. I'd either put this
and
> [_listAssets] back in serve.dart, or create a new file distinct from both the
> provider and the command that encapsulates.

Done. Reorganized code in serve.dart a bit too so it's not one giant onRun()
method.

[Bob Nystrom, 2013-08-15 23:48:31]

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