Emit useful information about build results #1710

samogot · 2018-08-04T01:37:47Z

Instead of just 'update' emit json with updated build results and its
digests

Instead of just 'update' emit json with updated build results and its digests

samogot · 2018-08-04T01:46:10Z

To find a way how to pass rootDir correctly was really hard. I was surprised that we can serve several different root dirs at the same time reusing the same handler (https://github.com/dart-lang/build/blob/hot-reload/build_runner/lib/src/entrypoint/serve.dart#L75). I wonder why we need several root dirs?

natebosch · 2018-08-06T18:30:39Z

build_runner/lib/src/server/path_to_asset_id.dart

+String assetIdToPath(AssetId assetId, String rootDir) =>
+    assetId.path.startsWith('lib/')
+        ? assetId.path.replaceFirst('lib/', 'packages/${assetId.package}/')
+        : assetId.path.replaceFirst('${rootDir}/', '');


Is this triggering a lint?

build/analysis_options.yaml

Line 76 in d59618a

- unnecessary_brace_in_string_interps

no, this doesn't compile without this braces. I don't understand why exactly, but looks like / is also tried to be interpreted somehow

I can't repro - what is the exact error you see?

Yeah, it works now. Looks like it was some weird error while trying different things.

natebosch · 2018-08-06T19:14:14Z

build_runner/lib/src/server/server.dart

-    for (var webSocket in activeConnections) {
-      webSocket.sink.add(_buildUpdatesMessage);
+  Future emitUpdateMessage(BuildResult buildResult) async {
+    if (buildResult.status == BuildStatus.success) {


[optional] Consider reversing the condition and bailing out early instead of nesting the entire method mode an extra level.

if (buildResult.status == BuildStatus.success) return; var digests = <AssetId, String>{}; ...

natebosch · 2018-08-06T19:16:46Z

build_runner/lib/src/server/server.dart

-      webSocket.sink.add(_buildUpdatesMessage);
+  Future emitUpdateMessage(BuildResult buildResult) async {
+    if (buildResult.status == BuildStatus.success) {
+      var digestMap = <AssetId, String>{};


[nit] how about digests or digestById? We typically avoid putting the collection type in the variable names.

natebosch · 2018-08-06T19:17:50Z

build_runner/lib/src/server/server.dart

+        var digest = await _state.reader.digest(assetId);
+        digestMap[assetId] = digest.toString();
+      }
+      for (var con in activeConnections) {


[nit] avoid the abbreviation, for (var connection in activeConnections)

natebosch · 2018-08-06T19:18:33Z

build_runner/lib/src/server/server.dart

+          var path = assetIdToPath(assetId, con.rootDir);
+          resultMap[path] = digestMap[assetId];
+        }
+        con.webSocket.sink.add(jsonEncode(resultMap));


this could be relatively big right? Should we worry about that?

I was more worried about body of request in other PR, and @jakemac53 said that it's ok.

natebosch · 2018-08-06T19:19:15Z

build_runner/lib/src/server/server.dart

@@ -123,36 +126,62 @@ class ServeHandler implements BuildState {
  }
 }

+class _ConnectionInfo {


how about _Connection. Info doesn't add new value

jakemac53 · 2018-08-06T14:14:30Z

build_runner/lib/src/server/server.dart

+      for (var con in activeConnections) {
+        var resultMap = <String, String>{};
+        for (var assetId in digestMap.keys) {
+          var path = assetIdToPath(assetId, con.rootDir);


Any asset not under con.rootDir or the lib dir can't be referenced anyways so we should skip them here.

jakemac53 · 2018-08-06T14:15:35Z

build_runner/lib/src/server/path_to_asset_id.dart

+String assetIdToPath(AssetId assetId, String rootDir) =>
+    assetId.path.startsWith('lib/')
+        ? assetId.path.replaceFirst('lib/', 'packages/${assetId.package}/')
+        : assetId.path.replaceFirst('${rootDir}/', '');


Similar to my comment below, but if it doesn't start with $rootDir or lib then we can't produce a valid path. This function could either throw or return null to indicate that, and we should document which it does.

- Filter out inaccessible paths - Group connections by root dir - Update shelf_web_socket module

jakemac53 · 2018-08-07T16:13:41Z

build_runner/test/server/serve_handler_test.dart

+        await clientChannel1.sink.close();
+      });
+
+      test('emmits only on sussesful builds', () async {


nit: successful

natebosch · 2018-08-07T16:50:22Z

build_runner/lib/src/server/server.dart

+  BuildUpdatesWebSocketHandler(this._state,
+      [this._handlerFactory = webSocketHandler]) {}
+
+  shelf.Handler getHandlerByRootDir(String rootDir) {


[nit] avoid naming methods with "get"

https://www.dartlang.org/guides/language/effective-dart/design#avoid-starting-a-method-name-with-get

natebosch · 2018-08-07T16:51:28Z

build_runner/test/server/serve_handler_test.dart

-      BuildUpdatesWebSocketHandler buildUpdatesWebSocketHandler;
-      Function createMockConection;
+      BuildUpdatesWebSocketHandler handler;
+      Future<Null> Function(WebSocketChannel, String) createMockConnection;


[nit] Prefer Future<void> over Future<Null>. The reason you see Future<Null> in so much existing code is void used to not work there, but now that it does it's our preference for new code

* Add assets digests handler (#1709) This entry point will be used for hot reloading, in order to initially retrieve digests of all assets, as build_runner might not know list of all of them, unlike client. * Emit useful information about build results (#1710) Instead of just 'update' emit json with updated build results and its digests * Move live reload client code to new hot_reload_client.dart file (#1732) It is expected that client code will become complicated, so in separate dart file it would be easier to test and maintain. * Implement basic hot reload (#1741) Reload any changed modules without any specific order, then reload main module and call main func to hopefully apply changes. Still need to get module dependency graph to reload in topological order and find correct ancessor chain where reloaded modules can be saved in closures. * Basic graph handling for hot reload (#1759) We still lack handling situations when graph is dynamically updated between reloads. * Add rough graph changes handling by full page reload (#1763) * Simplify module interface (#1764) * Change HMR API to work with several libraries bundled in one module (#1765) Fixes #1762 * Close Hot Reload listeners on exit (#1775) Closes #1774 * Refactor and improve HMR code (#1770) - Get rid of `roughLibraryKeyDecode` - use library path exposed from dart runtime now. - Get load modules from cache directly and throw if it isn't loaded instead of using require.js and hoping that module already loaded and no network request would be done. - Don't mess with `.ddc` extension in `build_runner` code - encapsulate all of it on build_web_compilers side. Fixes #1760 Fixes #1762 * Finalize hot-reloading feature (#1773) - Rename live-reload to hot-reload - Add debug logging about reloading - Add documentation - Version bump and changelog Fixes #1766 * Restore --live-reload option (#1778)

Emit useful information about build results

99eb549

Instead of just 'update' emit json with updated build results and its digests

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

samogot requested review from jakemac53 and natebosch August 4, 2018 01:37

natebosch reviewed Aug 6, 2018

View reviewed changes

jakemac53 reviewed Aug 6, 2018

View reviewed changes

Fix review comments and other optimizations

41ae528

- Filter out inaccessible paths - Group connections by root dir - Update shelf_web_socket module

jakemac53 approved these changes Aug 7, 2018

View reviewed changes

Update serve_handler_test.dart

5618a4c

natebosch reviewed Aug 7, 2018

View reviewed changes

Fix nits

b98cd25

natebosch approved these changes Aug 7, 2018

View reviewed changes

samogot merged commit e3a4fae into hot-reload Aug 7, 2018

samogot deleted the emit-build-results branch August 7, 2018 23:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Emit useful information about build results #1710

Emit useful information about build results #1710

samogot commented Aug 4, 2018

samogot commented Aug 4, 2018

natebosch Aug 6, 2018

samogot Aug 6, 2018

natebosch Aug 6, 2018

samogot Aug 7, 2018

natebosch Aug 6, 2018

natebosch Aug 6, 2018

natebosch Aug 6, 2018

natebosch Aug 6, 2018

samogot Aug 7, 2018

natebosch Aug 6, 2018

jakemac53 Aug 6, 2018

jakemac53 Aug 6, 2018

jakemac53 Aug 7, 2018

natebosch Aug 7, 2018

natebosch Aug 7, 2018

Emit useful information about build results #1710

Emit useful information about build results #1710

Conversation

samogot commented Aug 4, 2018

samogot commented Aug 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment