Implement basic hot reload #1741

samogot · 2018-08-09T21:03:16Z

Reload any changed modules without any specific order, than 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 accessor chain where reloaded modules can be saved in
closures.

Reload any changed modules without any specific order, than 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 accessor chain where reloaded modules can be saved in closures.

samogot · 2018-08-09T21:04:56Z

build_runner/test/server/hot_reload_client_test.dart

+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+@TestOn('browser')


Do I need to add any new things to .travis.yml for this test to be run?

Yes - we can talk offline :)

natebosch · 2018-08-09T21:38:55Z

build_runner/lib/src/server/hot_reload_client.dart

+List<String> get moduleUrls {
+  JsArray callMethod = context['Array']
+      .callMethod('from', [_urlToModuleId.callMethod('keys', [])]);
+  return List.castFrom(callMethod);


Will we potentially be accessing elements from this list multiple times? We probably want List.from instead of List.castFrom. The distinction is that castFrom is lazy whereas List.from eagerly casts each element once and then doesn't need to check types again later.
https://www.dartlang.org/guides/language/effective-dart/usage#avoid-using-cast

natebosch · 2018-08-09T21:40:51Z

build_runner/lib/src/server/hot_reload_client.dart

 import 'dart:html';
+import 'dart:js';


we generally prefer to use package:js instead of dart:js - can you see what it would look like doing that? It's possible not much will need to change since package:js exports parts of dart:js.

It don't exports JsObject and other types I use. And using @JS annotation will ends up with large amount of boilerplate code

JsObject is soft-deprecated:
dart-lang/sdk#33350

We will likely be adding lints in the near future to prevent usage:
dart-lang/sdk#33355

Using package:js/js_util.dart you should have access to some lower-level APIs that don't require much boiler-plate.

natebosch · 2018-08-09T21:42:50Z

build_runner/lib/src/server/hot_reload_client.dart

+    }
+    if (moduleIdsToReload.isNotEmpty) {
+      await Future.wait(moduleIdsToReload
+          .map((moduleId) => Future.value(_reloadModule(moduleId))));


why do we need the Future.value here?

Because Future.wait expects Future, not FutureOr. And mock implementation of _reloadModule returns plain value.

jakemac53 · 2018-08-09T21:33:52Z

build_runner/lib/src/server/hot_reload_client.dart

+  final FutureOr<JsObject> Function(String) _reloadModule;
+  final Map<String, String> _digests;
+
+  ReloadHandler(Map<String, String> this._digests,


you don't need to add the explicit type here, it will be the type of the field already (didn't even realize you could! haha)

jakemac53 · 2018-08-09T21:40:35Z

build_runner/lib/src/server/hot_reload_client.dart

+      this._reloadModule = reloadModule]);
+
+  void listener(MessageEvent e) async {
+    Map updatedAssetDigests = json.decode(e.data as String);


nit: use var on the left hand side and cast it to a Map explicitly:

var updatedAssetDigests = json.decode(e.data as String) as Map<String, dynamic>;

jakemac53 · 2018-08-09T21:41:57Z

build_runner/lib/src/server/hot_reload_client.dart

+  void listener(MessageEvent e) async {
+    Map updatedAssetDigests = json.decode(e.data as String);
+    var moduleIdsToReload = <String>[];
+    for (String path in updatedAssetDigests.keys) {


you can swap String for var here once you make the change above

jakemac53 · 2018-08-09T21:50:38Z

build_runner/lib/src/server/hot_reload_client.dart

+JsObject _urlToModuleId = _dartLoader['urlToModuleId'];
+
+List<String> get moduleUrls {
+  JsArray callMethod = context['Array']


nit: Instead of creating a new array on the javascript side, you can just use List.from to make a copy on the Dart side.

It doesn't work this way. JsObject isn't subtype of Iterable, and JS Iterator either

If you don't use dart:js you won't get a JsObject.

But there is no class in Dart I can directly map JS' Map or Iterator to. Or at least I failed to find one

I don't understand the problem. What are you trying to do?

How to get access to JS' Map's keys?

@JS(r'$dartLoader.urlToModuleId') external Map urlToModuleId;

Fails when I do urlToModuleId.keys (no some method with dart-mangled name)

@JS class JSIteratorItem { external bool get done; external dynamic get value; } @JS class JsIterator { external JSIteratorItem next(); } @JS(r'$dartLoader.urlToModuleId.keys') external JsIterator urlToModuleIdKeys();

Something like this should work, but it too verbose, and I need to iterate it manually.

I wish I don't understand something and there is a better way.

There is no such thing as a JS Map (well, other than ES6 Map, that is different).

Do you mean an object in JavaScript? If that is the case, you can write:

@JS(r'$dartLoader.urlToModuleId') external Object get urlToModuleId; @JS('Object.keys') external List<dynamic> _jsObjectKeys(Object any); List<String> jsObjectKeys(Object any) => List.from(_jsObjectKeys(any)); // Example use. void main() { var keys = jsObjectKeys(urlToModuleId); // List<String> }

No, I mean exactly ES6 Map

Ok, sorry about that. Then the following should work:

@JS(r'$dartLoader.urlToModuleId') external JsMap get urlToModuleId; @JS('Map') abstract class JsMap { external Object keys(); } @JS('Array.from') external List<dynamic> jsArrayFrom(Object any);

... does that help?

Yeah, thanks

jakemac53 · 2018-08-09T21:55:43Z

build_runner/lib/src/server/hot_reload_client.dart

+
+  var request = await HttpRequest.request('/$_assetsDigestPath',
+      responseType: 'json', sendData: modulePathsJson, method: 'POST');
+  var digests =


you can use .cast<String, String> on the original map instead which is a bit cleaner:

var digests = (request.response as Map).cast<String, String>();

jakemac53 · 2018-08-09T21:58:22Z

build_runner/test/server/hot_reload_client_test.dart

+    verifyZeroInteractions(methods);
+  });
+
+  test('do not reload random non-module files, but remember their digest', () {


should we remember the digests in this case? what would we do with them?

To handle new modules that may appear while developing. I'm not sure yet, if it will be possible to hot-load them, but if it will - we will need their digests to hot-reload them next time

grouma

Excited to see you making great progress :D

grouma · 2018-08-10T07:27:19Z

build_runner/test/server/hot_reload_client_test.dart

@@ -0,0 +1,86 @@
+// Copyright (c) 2017, the Dart project authors.  Please see the AUTHORS file


grouma · 2018-08-10T07:30:39Z

build_runner/lib/src/server/hot_reload_client.dart

+
+class ReloadHandler {
+  final String Function(String) _moduleIdByPath;
+  final FutureOr<Object> Function(String) _reloadModule;


Should this be a typedef to make it easier to mock? I find it a little odd in the test to have an abstract class of methods.

I haven't find any examples how to mock functions/typedefs directly outside of a class/interface. Also packing functions into class allows to use verifyZeroInteractions and verifyNoMoreInteractions

grouma · 2018-08-10T07:30:54Z

build_runner/lib/src/server/hot_reload_client.dart

+  return completer.future;
+}
+
+class ReloadHandler {


[nit] This should probably get a doc comment.

grouma · 2018-08-10T07:31:38Z

build_runner/lib/src/server/hot_reload_client.dart

+  return List.from(jsArrayFrom(dartLoader.urlToModuleId.keys()));
+}
+
+String moduleIdByPath(String path) =>


Does this need to be public? Same with moduleUrls?

grouma · 2018-08-10T07:33:02Z

build_runner/test/server/hot_reload_client_test.dart

+    verifyNoMoreInteractions(methods);
+  });
+
+  test('dropes .ddc suffix from module id', () async {


grouma · 2018-08-10T07:34:38Z

build_runner/lib/src/server/hot_reload_client.dart

-  webSocket.onMessage.listen((MessageEvent e) {
-    window.location.reload();
-  });
+  webSocket.onMessage.listen(handler.listener);


Not for this PR but do we need some error handling on this connection? Should we attempt to reconnect if there are issues. I can go into detail for what we do in DDR.

Yeah, I think we should. Hear more details will be great

jakemac53 · 2018-08-13T21:34:36Z

build_runner/pubspec.yaml

@@ -45,6 +45,8 @@ dev_dependencies:
  build_test: ^0.10.0
  build_modules: ^0.3.0
  build_web_compilers: ^0.4.0
+  js: ^0.6.1+1


This is imported from lib and so it should move to a real dependency.

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

samogot requested review from jakemac53 and natebosch August 9, 2018 21:03

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

samogot commented Aug 9, 2018

View reviewed changes

natebosch reviewed Aug 9, 2018

View reviewed changes

jakemac53 reviewed Aug 9, 2018

View reviewed changes

samogot added 2 commits August 9, 2018 17:07

Fix some nits

5b36702

Replace dart:js with package:js

deb0b2e

grouma reviewed Aug 10, 2018

View reviewed changes

Fix some nits

4e65d0b

samogot force-pushed the implement-basic-reload branch from 893a73b to 4e65d0b Compare August 10, 2018 23:46

Untie client tests from browser environment

3af752c

samogot force-pushed the implement-basic-reload branch from 19ddabb to 3af752c Compare August 11, 2018 01:42

jakemac53 reviewed Aug 13, 2018

View reviewed changes

Update pubspec.yaml

1361a8e

jakemac53 approved these changes Aug 13, 2018

View reviewed changes

samogot merged commit 58ed11a into hot-reload Aug 13, 2018

samogot deleted the implement-basic-reload branch August 13, 2018 23:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement basic hot reload #1741

Implement basic hot reload #1741

samogot commented Aug 9, 2018

samogot Aug 9, 2018

jakemac53 Aug 9, 2018

natebosch Aug 9, 2018

natebosch Aug 9, 2018

samogot Aug 9, 2018

matanlurey Aug 10, 2018

natebosch Aug 9, 2018

samogot Aug 9, 2018

jakemac53 Aug 9, 2018

jakemac53 Aug 9, 2018

jakemac53 Aug 9, 2018

jakemac53 Aug 9, 2018

samogot Aug 10, 2018

matanlurey Aug 10, 2018

samogot Aug 10, 2018 •

edited

Loading

matanlurey Aug 10, 2018

samogot Aug 10, 2018

matanlurey Aug 10, 2018

samogot Aug 10, 2018

matanlurey Aug 10, 2018

samogot Aug 10, 2018

jakemac53 Aug 9, 2018

jakemac53 Aug 9, 2018

samogot Aug 9, 2018

grouma left a comment

grouma Aug 10, 2018

grouma Aug 10, 2018

samogot Aug 10, 2018

grouma Aug 10, 2018

grouma Aug 10, 2018

grouma Aug 10, 2018

grouma Aug 10, 2018

samogot Aug 10, 2018

jakemac53 Aug 13, 2018

		@@ -0,0 +1,86 @@
		// Copyright (c) 2017, the Dart project authors. Please see the AUTHORS file

Implement basic hot reload #1741

Implement basic hot reload #1741

Conversation

samogot commented Aug 9, 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

samogot Aug 10, 2018 • edited Loading

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

grouma left a 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

samogot Aug 10, 2018 •

edited

Loading