Add simple live-reload implementation #1703

samogot · 2018-08-02T00:01:01Z

Add live-reload option that turns on simple WebSocket broadcaster of rebuild finished events.
Inject simple JS snippet to bootstrap that listen for update message from WebSocket and reloads the whole page.

In order to prepare for live/hot reload functionality in build_runner, add --live-reload option first.

In order to implement live/hot reload functionality in build_runner, add simple WebSocket that broadcasts a message each time rebuild finishes. For now clients should figure out what to do with it by themselves.

Add simple JS snippet to bootstrap code, to listen for WebSocket messages from server and reload the page on any changes.

jakemac53

Looks great! A few nitpicks but nothing big.

Before merging into master we will need to get some test coverage as well - but we could merge this as is into a separate branch while you work on tests if you would like.

Also, you will want to update the pubspec.yaml and CHANGELOG.md files to reflect the change. The new version should be 0.10.1-dev, we will remove the -dev when we are ready to release.

jakemac53 · 2018-08-02T13:49:01Z

build_runner/lib/src/server/server.dart


 const _performancePath = r'$perf';
 final _graphPath = r'$graph';
+final _hotReloadPath = r'$hotreload';


nit: lets name this $livereload for now at least

jakemac53 · 2018-08-02T13:52:51Z

build_runner/lib/src/server/server.dart

  final Future<AssetHandler> _assetHandler;
  final Future<AssetGraphHandler> _assetGraphHandler;

+  final WebSocketHandler _webSocketHandler = WebSocketHandler();


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

final _webSocketHandler = WebSocketHandler();

jakemac53 · 2018-08-02T13:54:06Z

build_runner/lib/src/server/server.dart

 }

+class WebSocketHandler {
+  final Set<WebSocketChannel> _connectionPull = Set();


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>();

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

jakemac53 · 2018-08-02T14:24:19Z

build_runner/lib/src/server/server.dart

+  final shelf.Handler _internalHandler;
+
+  // is it OK?
+  // ignore: implicit_this_reference_in_initializer


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); }

jakemac53 · 2018-08-02T15:36:36Z

build_web_compilers/lib/src/dev_compiler_bootstrap.dart

+    };
+  } 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


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.

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?

+1. This is my biggest concern

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.

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?

jakemac53 · 2018-08-02T15:39:09Z

build_runner/lib/src/server/server.dart

  }
 }

+class WebSocketHandler {


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.

# Conflicts: # build_runner/CHANGELOG.md # build_runner/pubspec.yaml

natebosch · 2018-08-02T00:04:35Z

build_runner/lib/src/server/server.dart

  final Future<AssetHandler> _assetHandler;
  final Future<AssetGraphHandler> _assetGraphHandler;

+  final WebSocketHandler _webSocketHandler = WebSocketHandler();


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

final _webSocketHandler = WebSocketHandler();

natebosch · 2018-08-02T17:15:58Z

build_runner/README.md


 - `--hostname`: The host to run the server on.

+- `--live-reload`: Enables automatic page reloading on rebuilds.


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

natebosch · 2018-08-02T17:20:10Z

build_runner/lib/src/server/server.dart

  final Future<AssetHandler> _assetHandler;
  final Future<AssetGraphHandler> _assetGraphHandler;

+  final _webSocketHandler = BuildUpdatesWebSocketHandler();


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?

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?

yes we could add unnecessary_new and unnecessary_const to avoid regressions.

natebosch · 2018-08-02T17:21:14Z

build_runner/lib/src/server/server.dart

  }
 }

+class BuildUpdatesWebSocketHandler {


a Doc comment would be good here

natebosch · 2018-08-02T17:22:17Z

build_web_compilers/lib/src/dev_compiler_bootstrap.dart

+    };
+  } 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


+1. This is my biggest concern

jakemac53 · 2018-08-02T21:59:22Z

build_runner/lib/src/server/server.dart

+    if (!request.url.path.endsWith('.js')) {
+      return innerHandler(request);
+    }
+    return Future.sync(() => innerHandler(request)).then((response) async {


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

jakemac53 · 2018-08-02T22:09:45Z

build_runner/lib/src/server/server.dart

+      // 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;


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

I don't think so.

Why do not remove comment - we might have other middlewares in future that will depend on the same extension point

Why append to the end of file - I feel it "safer" to append code to the end then to the begining

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.

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

natebosch · 2018-08-02T22:16:11Z

build_runner/lib/src/server/server.dart

+///
+/// Listen WebSocket for updates in build results
+///
+/// Now only ilve-reload functional - just reload page on update message


natebosch · 2018-08-02T22:21:25Z

build_runner/lib/src/server/server.dart

+      return innerHandler(request);
+    }
+    return Future.sync(() => innerHandler(request)).then((response) async {
+      // TODO: Find a way how to check and/or modify body without reading it whole


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.

jakemac53 · 2018-08-03T15:33:49Z

build_runner/lib/src/server/server.dart

 /// build updates
 class BuildUpdatesWebSocketHandler {
  final activeConnections = <WebSocketChannel>[];
+  final Function _handlerFactory;


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 :)

jakemac53 · 2018-08-03T15:50:29Z

build_runner/test/server/serve_handler_test.dart

  });
+
+  group('build updates', () {
+    final _entrypointExtensionMarker = '/* ENTRYPOINT_EXTENTION_MARKER */';


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.

jakemac53 · 2018-08-03T16:03:55Z

build_runner/test/server/serve_handler_test.dart

+    final _entrypointExtensionMarker = '/* ENTRYPOINT_EXTENTION_MARKER */';
+
+    test('injects client code if enabled', () async {
+      _addSource('a|web/some.js', _entrypointExtensionMarker);


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

jakemac53 · 2018-08-03T16:14:08Z

build_runner/test/server/serve_handler_test.dart

+      expect(await response.readAsString(), isNot(contains('\$livereload')));
+    });
+
+    test('emmits a message to all listners', () async {


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.

jakemac53 · 2018-08-03T16:16:50Z

build_runner/test/server/serve_handler_test.dart

+      var buildUpdatesWebSocketHandler = BuildUpdatesWebSocketHandler(
+          mockHandlerFactory);
+
+      var streamControllerFirst1 = StreamController<List<int>>();


nit: can we name these a bit more descriptively?

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

jakemac53 · 2018-08-03T16:18:47Z

build_runner/test/server/serve_handler_test.dart

+      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']));


Instead of this you can use the built in stream matchers

jakemac53 · 2018-08-03T18:08:19Z

build_runner/lib/src/server/server.dart

+  shelf.Handler _internalHandler;
+
+  BuildUpdatesWebSocketHandler([this._handlerFactory = webSocketHandler]) {
+    var untypedTearOff = (webSocket, protocol) =>


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

Created dart-archive/shelf_web_socket#11

jakemac53 · 2018-08-03T18:09:40Z

build_runner/lib/src/server/server.dart

+///
+/// Now only live-reload functional - just reload page on update message
+final _buildUpdatesInjectedJS = '''\n
+(function() {


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

jakemac53

lgtm, but lets give @natebosch a chance for final comments as well

samogot added 3 commits August 1, 2018 15:32

Add live-reload option

0c3d126

In order to prepare for live/hot reload functionality in build_runner, add --live-reload option first.

Add server-side WebSocket handler to inform clients about build state

867252e

In order to implement live/hot reload functionality in build_runner, add simple WebSocket that broadcasts a message each time rebuild finishes. For now clients should figure out what to do with it by themselves.

Implement simple live reloading on client side

c18faa2

Add simple JS snippet to bootstrap code, to listen for WebSocket messages from server and reload the page on any changes.

samogot requested review from jakemac53 and natebosch August 2, 2018 00:01

googlebot added the cla: yes Google is happy with the PR contributors label Aug 2, 2018

Reformat changes

abe6af9

jakemac53 reviewed Aug 2, 2018

View reviewed changes

samogot added 3 commits August 2, 2018 09:59

Fix review comments

60890f4

Update pubspec.yaml, CHANGELOG.md and README.md

f114ee5

Merge remote-tracking branch 'origin/master' into live-reload

12ceef4

# Conflicts: # build_runner/CHANGELOG.md # build_runner/pubspec.yaml

natebosch reviewed Aug 2, 2018

View reviewed changes

Move WebSocket client injection from build_web_compilers to build_runner

716ddcd

jakemac53 reviewed Aug 2, 2018

View reviewed changes

natebosch reviewed Aug 2, 2018

View reviewed changes

samogot added 2 commits August 2, 2018 16:50

Add tests

03569c5

Add more tests and reformat

7374c99

jakemac53 reviewed Aug 3, 2018

View reviewed changes

Improve tests

6a581ec

jakemac53 reviewed Aug 3, 2018

View reviewed changes

samogot added 2 commits August 3, 2018 11:28

Last nit comments

86ecfc8

Update the version of build_web_compilers

3c03e19

jakemac53 approved these changes Aug 3, 2018

View reviewed changes

natebosch approved these changes Aug 3, 2018

View reviewed changes

samogot merged commit 1ee129d into master Aug 4, 2018

samogot deleted the live-reload branch August 4, 2018 01:17


		- `--hostname`: The host to run the server on.

		- `--live-reload`: Enables automatic page reloading on rebuilds.

Add simple live-reload implementation #1703

Add simple live-reload implementation #1703

Uh oh!

Conversation

samogot commented Aug 2, 2018

Uh oh!

jakemac53 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakemac53 left a comment

Choose a reason for hiding this comment

Uh oh!