Skip to content
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions build_runner/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.10.1-dev

- Added `--live-reload` cli option and appropriate functionality

## 0.10.0

### Breaking Changes
Expand Down
1 change: 1 addition & 0 deletions build_runner/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ Some commands also have additional options:
##### serve

- `--hostname`: The host to run the server on.
- `--live-reload`: Enables automatic page reloading on rebuilds.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] Don't include an extra newline between bullet points.


Trailing args of the form `<directory>:<port>` are supported to customize what
directories are served, and on what ports.
Expand Down
4 changes: 4 additions & 0 deletions build_runner/lib/src/entrypoint/options.dart
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import 'package:path/path.dart' as p;
const assumeTtyOption = 'assume-tty';
const defineOption = 'define';
const deleteFilesByDefaultOption = 'delete-conflicting-outputs';
const liveReloadOption = 'live-reload';
const logPerformanceOption = 'log-performance';
const logRequestsOption = 'log-requests';
const lowResourcesModeOption = 'low-resources-mode';
Expand Down Expand Up @@ -130,11 +131,13 @@ class SharedOptions {
/// Options specific to the `serve` command.
class ServeOptions extends SharedOptions {
final String hostName;
final bool liveReload;
final bool logRequests;
final List<ServeTarget> serveTargets;

ServeOptions._({
@required this.hostName,
@required this.liveReload,
@required this.logRequests,
@required this.serveTargets,
@required bool assumeTty,
Expand Down Expand Up @@ -200,6 +203,7 @@ class ServeOptions extends SharedOptions {

return new ServeOptions._(
hostName: argResults[hostnameOption] as String,
liveReload: argResults[liveReloadOption] as bool,
logRequests: argResults[logRequestsOption] as bool,
serveTargets: serveTargets,
assumeTty: argResults[assumeTtyOption] as bool,
Expand Down
10 changes: 8 additions & 2 deletions build_runner/lib/src/entrypoint/serve.dart
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ class ServeCommand extends WatchCommand {
..addFlag(logRequestsOption,
defaultsTo: false,
negatable: false,
help: 'Enables logging for each request to the server.');
help: 'Enables logging for each request to the server.')
..addFlag(liveReloadOption,
defaultsTo: false,
negatable: false,
help: 'Enables automatic page reloading on rebuilds.');
}

@override
Expand Down Expand Up @@ -93,7 +97,9 @@ Future<HttpServer> _startServer(
ServeOptions options, ServeTarget target, ServeHandler handler) async {
var server = await _bindServer(options, target);
serveRequests(
server, handler.handlerFor(target.dir, logRequests: options.logRequests));
server,
handler.handlerFor(target.dir,
logRequests: options.logRequests, liveReload: options.liveReload));
return server;
}

Expand Down
90 changes: 82 additions & 8 deletions build_runner/lib/src/server/server.dart
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,18 @@ import 'package:logging/logging.dart';
import 'package:mime/mime.dart';
import 'package:path/path.dart' as p;
import 'package:shelf/shelf.dart' as shelf;
import 'package:shelf_web_socket/shelf_web_socket.dart';
import 'package:web_socket_channel/web_socket_channel.dart';

import '../generate/watch_impl.dart';
import 'asset_graph_handler.dart';
import 'path_to_asset_id.dart';

const _performancePath = r'$perf';
final _graphPath = r'$graph';
final _buildUpdatesProtocol = r'$livereload';
final _buildUpdatesMessage = 'update';
final _entrypointExtensionMarker = '/* ENTRYPOINT_EXTENTION_MARKER */';

final _logger = new Logger('Serve');

Expand All @@ -43,10 +48,13 @@ class ServeHandler implements BuildState {
final Future<AssetHandler> _assetHandler;
final Future<AssetGraphHandler> _assetGraphHandler;

final _webSocketHandler = BuildUpdatesWebSocketHandler();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're getting a little inconsistent about new (easy to do since the Analyzer no longer requires it)

@jakemac53 - should we dartfmt --fix and be done with it or should we find all of these and add back in the new?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am in favor of just doing a dartfmt --fix, but can we also enable a lint then to make us not introduce any more new keywords?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes we could add unnecessary_new and unnecessary_const to avoid regressions.


ServeHandler._(this._state, this._assetHandler, this._assetGraphHandler,
this._rootPackage) {
_state.buildResults.listen((result) {
_lastBuildResult = result;
_webSocketHandler.emitUpdateMessage(result);
});
}

Expand All @@ -55,14 +63,17 @@ class ServeHandler implements BuildState {
@override
Stream<BuildResult> get buildResults => _state.buildResults;

shelf.Handler handlerFor(String rootDir, {bool logRequests}) {
shelf.Handler handlerFor(String rootDir,
{bool logRequests, bool liveReload}) {
liveReload ??= false;
logRequests ??= false;
if (p.url.split(rootDir).length != 1) {
throw new ArgumentError.value(
rootDir, 'rootDir', 'Only top level directories are supported');
}
_state.currentBuild.then((_) => _warnForEmptyDirectory(rootDir));
var cascade = new shelf.Cascade()
var cascade = new shelf.Cascade();
cascade = (liveReload ? cascade.add(_webSocketHandler.handler) : cascade)
.add(_blockOnCurrentBuild)
.add((shelf.Request request) async {
if (request.url.path == _performancePath) {
Expand All @@ -76,12 +87,14 @@ class ServeHandler implements BuildState {
var assetHandler = await _assetHandler;
return assetHandler.handle(request, rootDir);
});
var handler = logRequests
? const shelf.Pipeline()
.addMiddleware(_logRequests)
.addHandler(cascade.handler)
: cascade.handler;
return handler;
var pipeline = shelf.Pipeline();
if (logRequests) {
pipeline = pipeline.addMiddleware(_logRequests);
}
if (liveReload) {
pipeline = pipeline.addMiddleware(_injectBuildUpdatesClientCode);
}
return pipeline.addHandler(cascade.handler);
}

Future<shelf.Response> _blockOnCurrentBuild(_) async {
Expand Down Expand Up @@ -110,6 +123,67 @@ class ServeHandler implements BuildState {
}
}

/// Class that manages web socket connection handler to inform clients about
/// build updates
class BuildUpdatesWebSocketHandler {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a Doc comment would be good here

final activeConnections = <WebSocketChannel>[];
final Function _handlerFactory;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets type this more strongly if possible, something like

Handler Function(void Function(WebSocket, String), {List<String> protocols})

A bit verbose to be sure, but nicely typed :)

shelf.Handler _internalHandler;

BuildUpdatesWebSocketHandler([this._handlerFactory = webSocketHandler]) {
var untypedTearOff = (webSocket, protocol) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one last nit, can you document why we need to do this, and ideally point at an issue in the original package?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_handleConnection(webSocket as WebSocketChannel, protocol as String);
_internalHandler = _handlerFactory(
untypedTearOff, protocols: [_buildUpdatesProtocol]) as shelf.Handler;
}

shelf.Handler get handler => _internalHandler;

void emitUpdateMessage(BuildResult buildResult) {
for (var webSocket in activeConnections) {
webSocket.sink.add(_buildUpdatesMessage);
}
}

void _handleConnection(WebSocketChannel webSocket, String protocol) async {
activeConnections.add(webSocket);
await webSocket.stream.drain();
activeConnections.remove(webSocket);
}
}

shelf.Handler _injectBuildUpdatesClientCode(shelf.Handler innerHandler) {
return (shelf.Request request) async {
if (!request.url.path.endsWith('.js')) {
return innerHandler(request);
}
var response = await innerHandler(request);
// TODO: Find a way how to check and/or modify body without reading it whole
var body = await response.readAsString();
if (body.startsWith(_entrypointExtensionMarker)) {
body += _buildUpdatesInjectedJS;
}
return response.change(body: body);
};
}

/// Hot-reload config
///
/// Listen WebSocket for updates in build results
///
/// Now only live-reload functional - just reload page on update message
final _buildUpdatesInjectedJS = '''\n
(function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh also its probably worth adding a comment here // Injected by build_runner for live reload support or something along those lines

var ws = new WebSocket('ws://' + location.host, ['$_buildUpdatesProtocol']);
ws.onmessage = function(event) {
console.log(event);
if(event.data === '$_buildUpdatesMessage'){
location.reload();
}
};
}());
''';

class AssetHandler {
final FinalizedReader _reader;
final String _rootPackage;
Expand Down
4 changes: 3 additions & 1 deletion build_runner/pubspec.yaml
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
name: build_runner
version: 0.10.0
version: 0.10.1-dev
description: Tools to write binaries that run builders.
author: Dart Team <[email protected]>
homepage: https://github.com/dart-lang/build/tree/master/build_runner
Expand Down Expand Up @@ -34,9 +34,11 @@ dependencies:
pub_semver: ^1.4.0
pubspec_parse: ^0.1.0
shelf: ">=0.6.5 <0.8.0"
shelf_web_socket: ^0.2.2+3
stack_trace: ^1.9.0
stream_transform: ^0.0.9
watcher: ^0.9.7
web_socket_channel: ^1.0.9
yaml: ^2.1.0

dev_dependencies:
Expand Down
93 changes: 93 additions & 0 deletions build_runner/test/server/serve_handler_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import 'dart:io';

import 'package:logging/logging.dart';
import 'package:shelf/shelf.dart';
import 'package:stream_channel/stream_channel.dart';
import 'package:test/test.dart';

import 'package:build_runner/build_runner.dart';
Expand All @@ -20,6 +21,7 @@ import 'package:build_runner/src/server/server.dart';

import 'package:_test_common/common.dart';
import 'package:_test_common/package_graphs.dart';
import 'package:web_socket_channel/web_socket_channel.dart';

void main() {
ServeHandler serveHandler;
Expand Down Expand Up @@ -162,6 +164,97 @@ void main() {
expect(await response.readAsString(), contains('--track-performance'));
});
});

group('build updates', () {
final _entrypointExtensionMarker = '/* ENTRYPOINT_EXTENTION_MARKER */';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: you can import from the src directory and use the actual original definition of this (its fine to make it public there as long as it isn't exported by lib/build_runner.dart). Or, we could intentionally expose it so that build_web_compilers can use it.


test('injects client code if enabled', () async {
_addSource('a|web/some.js', _entrypointExtensionMarker);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: append some other js content to the end as well

var response = await serveHandler.handlerFor('web', liveReload: true)(
new Request('GET', Uri.parse('http://server.com/some.js')));
expect(await response.readAsString(), contains('\$livereload'));
});

test('doesn\'t inject client code if disabled', () async {
_addSource('a|web/some.js', _entrypointExtensionMarker);
var response = await serveHandler.handlerFor('web', liveReload: false)(
new Request('GET', Uri.parse('http://server.com/some.js')));
expect(await response.readAsString(), isNot(contains('\$livereload')));
});

test('doesn\'t inject client code in non-js files', () async {
_addSource('a|web/some.html', _entrypointExtensionMarker);
var response = await serveHandler.handlerFor('web', liveReload: true)(
new Request('GET', Uri.parse('http://server.com/some.html')));
expect(await response.readAsString(), isNot(contains('\$livereload')));
});

test('doesn\'t inject client code in non-marked files', () async {
_addSource('a|web/some.js', 'content');
var response = await serveHandler.handlerFor('web', liveReload: true)(
new Request('GET', Uri.parse('http://server.com/some.js')));
expect(await response.readAsString(), isNot(contains('\$livereload')));
});

test('emmits a message to all listners', () async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this test and the next one share a lot of setup logic, you can move them into a group (see 2nd code snippet here).

That lets you define a shared setUp and tearDown function for both tests.

Function exposedOnConnect;
var mockHandlerFactory = (Function onConnect, {protocols}) {
exposedOnConnect = onConnect;
};
var buildUpdatesWebSocketHandler = BuildUpdatesWebSocketHandler(
mockHandlerFactory);

var streamControllerFirst1 = StreamController<List<int>>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we name these a bit more descriptively?

It is also important to make sure these get closed (in a tearDown method ideally).

var streamControllerFirst2 = StreamController<List<int>>();
var serverChannelFirst = WebSocketChannel(StreamChannel(streamControllerFirst1.stream, streamControllerFirst2.sink), serverSide: true);
var clientChannelFirst = WebSocketChannel(StreamChannel(streamControllerFirst2.stream, streamControllerFirst1.sink), serverSide: false);

var streamControllerSecond1 = StreamController<List<int>>();
var streamControllerSecond2 = StreamController<List<int>>();
var serverChannelSecond = WebSocketChannel(StreamChannel(streamControllerSecond1.stream, streamControllerSecond2.sink), serverSide: true);
var clientChannelSecond = WebSocketChannel(StreamChannel(streamControllerSecond2.stream, streamControllerSecond1.sink), serverSide: false);

var deferredExpect1 = clientChannelFirst.stream.single.then((value) => expect(value, 'update'));
var deferredExpect2 = clientChannelSecond.stream.single.then((value) => expect(value, 'update'));
exposedOnConnect(serverChannelFirst, '');
exposedOnConnect(serverChannelSecond, '');
buildUpdatesWebSocketHandler.emitUpdateMessage(null);
await clientChannelFirst.sink.close();
await clientChannelSecond.sink.close();
await deferredExpect1;
await deferredExpect2;
});

test('deletes listners on disconect', () async {
Function exposedOnConnect;
var mockHandlerFactory = (Function onConnect, {protocols}) {
exposedOnConnect = onConnect;
};
var buildUpdatesWebSocketHandler = BuildUpdatesWebSocketHandler(
mockHandlerFactory);

var streamControllerFirst1 = StreamController<List<int>>();
var streamControllerFirst2 = StreamController<List<int>>();
var serverChannelFirst = WebSocketChannel(StreamChannel(streamControllerFirst1.stream, streamControllerFirst2.sink), serverSide: true);
var clientChannelFirst = WebSocketChannel(StreamChannel(streamControllerFirst2.stream, streamControllerFirst1.sink), serverSide: false);

var streamControllerSecond1 = StreamController<List<int>>();
var streamControllerSecond2 = StreamController<List<int>>();
var serverChannelSecond = WebSocketChannel(StreamChannel(streamControllerSecond1.stream, streamControllerSecond2.sink), serverSide: true);
var clientChannelSecond = WebSocketChannel(StreamChannel(streamControllerSecond2.stream, streamControllerSecond1.sink), serverSide: false);

var deferredExpect1 = clientChannelFirst.stream.toList().then((value) => expect(value, ['update', 'update']));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of this you can use the built in stream matchers

var deferredExpect2 = clientChannelSecond.stream.single.then((value) => expect(value, 'update'));
exposedOnConnect(serverChannelFirst, '');
exposedOnConnect(serverChannelSecond, '');
buildUpdatesWebSocketHandler.emitUpdateMessage(null);
await clientChannelSecond.sink.close();
buildUpdatesWebSocketHandler.emitUpdateMessage(null);
await clientChannelFirst.sink.close();
await deferredExpect1;
await deferredExpect2;
});
});
}

class MockWatchImpl implements WatchImpl {
Expand Down
8 changes: 7 additions & 1 deletion build_web_compilers/lib/src/dev_compiler_bootstrap.dart
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ Future<Null> bootstrapDdc(BuildStep buildStep,
: _context.joinAll(_context.split(jsId.path).skip(1)));
}

var bootstrapContent = new StringBuffer('(function() {\n');
var bootstrapContent = new StringBuffer('$_entrypointExtensionMarker\n(function() {\n');
bootstrapContent.write(_dartLoaderSetup(modulePaths));
bootstrapContent.write(_requireJsConfig);

Expand Down Expand Up @@ -321,6 +321,12 @@ require.config({
});
''';

/// Marker comment used by build_runner (or any other thinks) to identify
/// entrypoint file, to inject custom code there.
///
/// Should be first line in a file, so server don't need to parse whole body
final _entrypointExtensionMarker = '/* ENTRYPOINT_EXTENTION_MARKER */';

final _baseUrlScript = '''
var baseUrl = (function () {
// Attempt to detect base url using <base href> html tag
Expand Down