Basic graph handling for hot reload #1759

samogot · 2018-08-14T01:49:12Z

Fixes #1743
We still lack handling situations when graph is dynamically updated
between reloads.

We still lack handling situations when graph is dynamically updated between reloads.

jakemac53

We will need some docs on the hooks that are available and how exactly frameworks/users can take advantage of them. You could add a new markdown file under the docs/ folder and link to it from the readme.

Note that we actually are guaranteed that there are no cycles in the module graph by the build_modules package, although it doesn't hurt to have the bit of supporting logic you have in here since it is relatively trivial.

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

build_runner/lib/src/server/hot_reload_client/reloading_manager.dart

+  SplayTreeSet<String> _dirty;
+  Completer<void> _running = Completer()..complete();
+
+  int moduleTopologicalCompare(String module1, String module2) {


you could make this a static or top level method, and then you can reference it in the initializer list of the constructor so that _dirty can be final.

I can't, because it references to instance field _moduleOrdering.

ah I missed that somehow, sgtm

jakemac53 · 2018-08-14T14:17:46Z

build_runner/lib/src/server/hot_reload_client/reloading_manager.dart

+  final Iterable<String> Function() _allModules;
+
+  final Map<String, int> _moduleOrdering = {};
+  SplayTreeSet<String> _dirty;


nit: _dirtyModules or something just a bit more descriptive

jakemac53 · 2018-08-14T14:20:27Z

build_runner/lib/src/server/hot_reload_client/reloading_manager.dart

+    _dirty = SplayTreeSet(moduleTopologicalCompare);
+  }
+
+  Future<void> run(List<String> modules) async {


lets rename this to reload or something along those lines (it isn't entirely clear what run would do just from the name)

jakemac53 · 2018-08-14T14:36:24Z

build_runner/lib/src/server/hot_reload_client/reloading_manager.dart

+      var moduleId = _dirty.first;
+      _dirty.remove(moduleId);
+
+      var existing = await _loadModule(moduleId);


can this fail?

I assume that it should neither fail, nor make any network requests, and should return running module from cache.
This assumption might be wrong with dynamic graph updates, but it isn't handled anywhere yet anyway

Can we detect that situation and throw?

I'm not sure what should I detect right here. If for example module is no lodger exists, but it still left in the graph - require.js will throw here.
I feel it's better just to treat any graph changes as undefined behavior right now. I will explore how it can be handled (at least on level of detection and firing page reload) in next PR

This PR is already to complicated to continue major work in it.

jakemac53 · 2018-08-14T14:37:26Z

build_runner/lib/src/server/hot_reload_client/reloading_manager.dart

+      Map data;
+      if (existing.hasOnDestroy) {
+        data = {};
+        existing.onDestroy(data);


It seems like it might be better if we just allow this method to return a Map instead of modifying one its given

Might be. In this case it may be anything, not only Map. I was trying to make design more compatible with webpack hot reloading api, but it isn't the same anyway

We are passing in a map though right now, making it a return value would actually allow us to support dynamic or Object, which is more flexible.

jakemac53 · 2018-08-14T14:40:04Z

build_runner/lib/src/server/hot_reload_client/reloading_manager.dart

+      var newVersion = await _reloadModule(moduleId);
+      bool success;
+      if (newVersion.hasOnSelfUpdate) {
+        success = newVersion.onSelfUpdate(data);


It would probably be better to pass an empty map here in the case where onDestroy isn't implemented, instead of null (generally we try not to pass nulls for required args). Another option would be changing it to an optional argument.

jakemac53 · 2018-08-14T15:21:40Z

build_runner/lib/src/server/hot_reload_client/reloading_manager.dart

+      if (newVersion.hasOnSelfUpdate) {
+        success = newVersion.onSelfUpdate(data);
+      }
+      if (success == true) continue;


How about we change this to return an Enum describing the possible states instead of treating null in a special way. In general we try to avoid apis that have a return type but might actually return null in Dart (interestingly I don't see a line item about this in effective dart, but its not a common pattern).

I thought about it. But where we should declare this enum to use it in modules, and how to access to it from raw js code in bootstap?

Since you need access on the JS side I think you would have to make an enum-like object in JS, and then you can access that via package:js.

Essentially, in JS you would have some global object available that looks like this:

someGlobalScope.MyEnum = { 'bar': Symbol('bar'), 'baz': Symbol('baz') };

And then you can convert that to a Dart class with static getters like this:

@JS('someGlobalScope.MyEnum ') abstract class MyEnum { external static int get bar; external static int get baz; }

Sounds good, but where to place this JS. It will be a part of public api, so I feel it a bad idea to put it inside bootstrap file

We should be able to inject it in the same place we inject the other hot reloading code (based on the entrypoint marker?)

jakemac53 · 2018-08-14T15:28:43Z

build_runner/lib/src/server/hot_reload_client/reloading_manager.dart

+      }
+
+      var parentIds = _moduleParents(moduleId);
+      if (parentIds == null || parentIds.isEmpty) {


What real-world use case is this handling? It would be good to leave a comment here to describe it.

If we use something different than bootstrap file, we might have no hooks implementations in whole chain. In this case we should reload whole page. We have a test for it

Ah ok, so if we bubble up to the root then refresh - makes sense.

jakemac53 · 2018-08-14T15:50:51Z

build_web_compilers/lib/src/dev_compiler_bootstrap.dart

@@ -324,6 +336,16 @@ require.config({
    waitSeconds: 0,
    paths: customModulePaths
 });
+
+requirejs.onResourceLoad = function (context, map, depArray) {
+  // TODO Handle dynamic changes in module dependancies


This may already be handling this (if this function is invoked whenever new modules are loaded)?

New - might be. But deleting or changing parent - no. Currently graph is newer invalidated, that may cause cycles in it and who knows what other weird behavior.

samogot · 2018-08-14T16:29:40Z

We will need some docs on the hooks that are available and how exactly frameworks/users can take advantage of them. You could add a new markdown file under the docs/ folder and link to it from the readme.

I not feel this api is stabilized yet. I need to try to implement something in client modules first. I anticipate problems with name mangling if I'd try to just export functions with such names from dart code. I will write the docs when it's ready. For now there are doc comments to don't forget what it should do.

jakemac53

Lets create issues and assign them to the HotRestart github project to track any ongoing work that will be addressed in future PR's, and then we can link them in TODO's as well.

jakemac53 · 2018-08-14T17:02:47Z

build_runner/lib/src/server/hot_reload_client/reloading_manager.dart

+      Map data;
+      if (existing.hasOnDestroy) {
+        data = {};
+        existing.onDestroy(data);


We are passing in a map though right now, making it a return value would actually allow us to support dynamic or Object, which is more flexible.

jakemac53 · 2018-08-14T17:43:08Z

build_runner/lib/src/server/hot_reload_client/reloading_manager.dart

+      if (newVersion.hasOnSelfUpdate) {
+        success = newVersion.onSelfUpdate(data);
+      }
+      if (success == true) continue;


Since you need access on the JS side I think you would have to make an enum-like object in JS, and then you can access that via package:js.

Essentially, in JS you would have some global object available that looks like this:

someGlobalScope.MyEnum = { 'bar': Symbol('bar'), 'baz': Symbol('baz') };

And then you can convert that to a Dart class with static getters like this:

@JS('someGlobalScope.MyEnum ') abstract class MyEnum { external static int get bar; external static int get baz; }

jakemac53 · 2018-08-14T17:49:53Z

build_runner/lib/src/server/hot_reload_client/reloading_manager.dart

+      }
+
+      var parentIds = _moduleParents(moduleId);
+      if (parentIds == null || parentIds.isEmpty) {


Ah ok, so if we bubble up to the root then refresh - makes sense.

jakemac53

LGTM I think this is at a good enough point to submit for now and we have issues filed to track future work that needs to happen.

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

Basic graph handling for hot reload

0cc388a

We still lack handling situations when graph is dynamically updated between reloads.

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

samogot requested review from natebosch and jakemac53 August 14, 2018 01:52

jakemac53 reviewed Aug 14, 2018

View reviewed changes

samogot added 2 commits August 14, 2018 09:51

Some review suggestions

9e1f58c

Fix dev_web_compiler test

2aba858

jakemac53 reviewed Aug 14, 2018

View reviewed changes

jakemac53 approved these changes Aug 14, 2018

View reviewed changes

samogot merged commit 76b8be4 into hot-reload Aug 14, 2018

samogot deleted the hot-reload-graph branch August 14, 2018 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Basic graph handling for hot reload #1759

Basic graph handling for hot reload #1759

samogot commented Aug 14, 2018

jakemac53 left a comment

jakemac53 Aug 14, 2018

samogot Aug 14, 2018

jakemac53 Aug 14, 2018

jakemac53 Aug 14, 2018

jakemac53 Aug 14, 2018

jakemac53 Aug 14, 2018

samogot Aug 14, 2018

jakemac53 Aug 14, 2018

samogot Aug 14, 2018

samogot Aug 14, 2018

jakemac53 Aug 14, 2018

samogot Aug 14, 2018

jakemac53 Aug 14, 2018

jakemac53 Aug 14, 2018

jakemac53 Aug 14, 2018

samogot Aug 14, 2018

jakemac53 Aug 14, 2018

samogot Aug 14, 2018

jakemac53 Aug 14, 2018

jakemac53 Aug 14, 2018

samogot Aug 14, 2018

jakemac53 Aug 14, 2018

jakemac53 Aug 14, 2018

samogot Aug 14, 2018

samogot commented Aug 14, 2018

jakemac53 left a comment

jakemac53 Aug 14, 2018

jakemac53 Aug 14, 2018

jakemac53 Aug 14, 2018

jakemac53 left a comment

Basic graph handling for hot reload #1759

Basic graph handling for hot reload #1759

Conversation

samogot commented Aug 14, 2018

jakemac53 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

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

samogot commented Aug 14, 2018

jakemac53 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

jakemac53 left a comment

Choose a reason for hiding this comment