Skip to content

Add simple live-reload implementation #1703

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Aug 4, 2018
Merged

Add simple live-reload implementation #1703

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
0c3d126
Add live-reload option
samogot Aug 1, 2018
867252e
Add server-side WebSocket handler to inform clients about build state
samogot Aug 1, 2018
c18faa2
Implement simple live reloading on client side
samogot Aug 1, 2018
abe6af9
Reformat changes
samogot Aug 2, 2018
60890f4
Fix review comments
samogot Aug 2, 2018
f114ee5
Update pubspec.yaml, CHANGELOG.md and README.md
samogot Aug 2, 2018
12ceef4
Merge remote-tracking branch 'origin/master' into live-reload
samogot Aug 2, 2018
716ddcd
Move WebSocket client injection from build_web_compilers to build_runner
samogot Aug 2, 2018
03569c5
Add tests
samogot Aug 2, 2018
7374c99
Add more tests and reformat
samogot Aug 3, 2018
6a581ec
Improve tests
samogot Aug 3, 2018
86ecfc8
Last nit comments
samogot Aug 3, 2018
3c03e19
Update the version of build_web_compilers
samogot Aug 3, 2018
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/lib/src/entrypoint/options.dart
View file
Open in desktop
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
8 changes: 6 additions & 2 deletions build_runner/lib/src/entrypoint/serve.dart
View file
Open in desktop
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,7 @@ 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
40 changes: 38 additions & 2 deletions build_runner/lib/src/server/server.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,16 @@ 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 _hotReloadPath = r'$hotreload';
Copy link
Contributor

@jakemac53 jakemac53 Aug 2, 2018

Choose a reason for hiding this comment

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

nit: lets name this $livereload for now at least


final _logger = new Logger('Serve');

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

final WebSocketHandler _webSocketHandler = WebSocketHandler();
Copy link
Member

@natebosch natebosch Aug 2, 2018

Choose a reason for hiding this comment

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

You can skip the type on the left when it's obvious on the right.

final _webSocketHandler = WebSocketHandler();

Copy link
Contributor

@jakemac53 jakemac53 Aug 2, 2018

Choose a reason for hiding this comment

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

nit: you should omit the type on the left hand side here, inference will take care of it :)

final _webSocketHandler = WebSocketHandler();


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

Expand All @@ -55,14 +61,16 @@ 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 Down Expand Up @@ -110,6 +118,34 @@ class ServeHandler implements BuildState {
}
}

class WebSocketHandler {
Copy link
Contributor

@jakemac53 jakemac53 Aug 2, 2018

Choose a reason for hiding this comment

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

This is pretty specific to LiveReload right now, we might make it more generic later but for now lets rename it to LiveReloadWebSocketHandler - or maybe BuildUpdatesWebSocketHandler.

final Set<WebSocketChannel> _connectionPull = Set();
Copy link
Contributor

@jakemac53 jakemac53 Aug 2, 2018

Choose a reason for hiding this comment

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

similar to above, you can omit the type on the left and add the generic to the constructor on the right:

final _connectionPull = Set<WebSocketChannel>();

Copy link
Contributor

@jakemac53 jakemac53 Aug 2, 2018

Choose a reason for hiding this comment

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

This can be a List instead of a Set (<WebSocketChannel>[]), and I would rename it to connections or activeConnections.

final shelf.Handler _internalHandler;

// is it OK?
// ignore: implicit_this_reference_in_initializer
Copy link
Contributor

@jakemac53 jakemac53 Aug 2, 2018

Choose a reason for hiding this comment

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

I am surprised this wouldn't give a compile time error in the CFE - that is probably a bug.

We will need to handle this in a different way as it is supposed to be an error. One way is to move it into the constructor body (but then it can't be final):

shelf.Handler _internalHandler;

WebSocketHandler() {
    _internalHandler = webSocketHandler(_handleConnection);
}

The other way would be to use a public factory that delegates to a private constructor, and a static _handleConnection method that takes the connection set, although it's a lot more boilerplate:

factory WebSocketHandler() => WebSocketHandler._(new Set<WebSocketChannel>());

WebSocketHandler._(this._connectionPull) :
    _internalHandler = webSocketHandler((socket) => _handleConnection(socket, _connectionPull));

static void _handleConnection(
    WebSocketChannel webSocket, Set<WebSocketChannel> connections) async {
  connections.add(webSocket);
  await webSocket.stream.drain();
  connections.remove(webSocket);
}

WebSocketHandler() : _internalHandler = webSocketHandler(_handleConnection);

Future<shelf.Response> handler(shelf.Request request) async {
if (!request.url.path.startsWith(_hotReloadPath)) {
return new shelf.Response.notFound('');
}
return _internalHandler(request);
}

void emitUpdateMessage(BuildResult buildResult) {
for (var webSocket in _connectionPull) {
webSocket.sink.add('update');
}
}

void _handleConnection(WebSocketChannel webSocket) async {
_connectionPull.add(webSocket);
await webSocket.stream.drain();
_connectionPull.remove(webSocket);
}
}

class AssetHandler {
final FinalizedReader _reader;
final String _rootPackage;
Expand Down
2 changes: 2 additions & 0 deletions build_runner/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
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
31 changes: 31 additions & 0 deletions build_web_compilers/lib/src/dev_compiler_bootstrap.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ Future<Null> bootstrapDdc(BuildStep buildStep,
var bootstrapContent = new StringBuffer('(function() {\n');
bootstrapContent.write(_dartLoaderSetup(modulePaths));
bootstrapContent.write(_requireJsConfig);
bootstrapContent.write(_hotReloadConfig);

bootstrapContent.write(_appBootstrap(
appModuleName, appModuleScope, ignoreCastFailures, enableSyncAsync));
Expand Down Expand Up @@ -321,6 +322,36 @@ require.config({
});
''';

/// Hot-reload config
///
/// Listen WebSocket for updates in build results
///
/// Now only ilve-reload functional - just reload page on update message
final _hotReloadConfig = '''
(function() {
var wsUrl;
if (baseUrl.startsWith('http')) {
wsUrl = baseUrl.replace('http', 'ws');
} else {
// TODO: when it may make sense? WebWorkers? Tests?
wsUrl = 'ws://' + location.host + baseUrl;
}
wsUrl += '\$hotreload';
try {
var ws = new WebSocket(wsUrl);
ws.onmessage = function(event) {
console.log(event);
if(event.data === 'update'){
location.reload();
}
};
} catch (e) {
// Live reload might be turned off. Just ignore it for now
// TODO: Find a way to pass serve option here, to not inject this code at all, if live/hot reload is turned off
Copy link
Contributor

@jakemac53 jakemac53 Aug 2, 2018

Choose a reason for hiding this comment

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

This is an interesting problem - ideally this package does not have to rely on the implementation details in the server (such as the update event, or the $hotreload path). We want to keep breaking changes in build_runner to a minimum, so ideally we wouldn't have other packages depending on these details.

To that end, it might make sense to actually inject this code from the server itself, possibly into all html requests that it serves, although that could be challenging as well. It would be worth talking to @grouma about how we do this for DDR internally.

Copy link
Contributor Author

@samogot samogot Aug 2, 2018

Choose a reason for hiding this comment

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

It looks like non-trivial task to figure out if request is html page or not. Both Content-type header and <html> tag might be omitted.
Maybe it's easier to inject code to the end of any *.dart.bootstrap.js file?

Copy link
Member

@natebosch natebosch Aug 2, 2018

Choose a reason for hiding this comment

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

+1. This is my biggest concern

Copy link
Contributor

@jakemac53 jakemac53 Aug 2, 2018

Choose a reason for hiding this comment

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

Maybe it's easier to inject code to the end of any *.dart.bootstrap.js file?

The problem with that is we are then backing knowledge of build_web_compilers into build_runner - which is maybe worse than the other way around haha.

Copy link
Member

@grouma grouma Aug 3, 2018

Choose a reason for hiding this comment

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

DDR knows ahead of time what the main entry JS file is called. It then intercepts this request and returns the DDR application. This application will embed a request for the main entry along with a request parameter forReal. The logic can be found in ddr_handler.dart. This pattern has been helpful because we have completely changed our injected application logic a number of times. Is it possible to know what the root request will be in build_runner?

}
}());
''';

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