Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 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
87 changes: 79 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,64 @@ 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>[];
shelf.Handler _internalHandler;

BuildUpdatesWebSocketHandler() {
_internalHandler = webSocketHandler(_handleConnection, protocols: [_buildUpdatesProtocol]);
}

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) {
if (!request.url.path.endsWith('.js')) {
return innerHandler(request);
}
return Future.sync(() => innerHandler(request)).then((response) async {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that you can await a FutureOr (or any synchronous value actually), which could clean this us just a bit.

// TODO: Find a way how to check and/or modify body without reading it whole
Copy link
Member

Choose a reason for hiding this comment

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

This is probably ok since most .js is going to be UTF8 encoded. We could also consider implementing a StreamTransformer that would do this but it's probably more complex that is warranted.

var body = await response.readAsString();
if (body.startsWith(_entrypointExtensionMarker)) {
body += _buildUpdatesInjectedJS;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should replace the placeholder comment as opposed to appending

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so.

  1. Why do not remove comment - we might have other middlewares in future that will depend on the same extension point
  2. Why append to the end of file - I feel it "safer" to append code to the end then to the begining

Copy link
Member

Choose a reason for hiding this comment

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

What is the browser behavior if there is some valid JS source followed by invalid stuff - will the stuff at the beginning of the file still get executed?
In our particular setup we're pretty sure we're always sending valid parseable javascript, but if the browser would start running our injected stuff even if the rest of the file contains broken stuff that could be a reason to put it at the beginning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICR if there is fatal syntax error - it won't. if "invalid stuff" can somehow be parsed as JS, then file will be executed till the first unhanded error

}
return response.change(body: body);
});
};
}

/// Hot-reload config
///
/// Listen WebSocket for updates in build results
///
/// Now only ilve-reload functional - just reload page on update message
Copy link
Member

Choose a reason for hiding this comment

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

[nit] typo

final _buildUpdatesInjectedJS = '''
(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
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