Change HMR API to work with several libraries bundled in one module #1765
Conversation
| void Function(HotReloadableModule module) callback, | ||
| void Function(JsError e) onError); | ||
| external void forceLoadModule(String moduleId, | ||
| void Function(Object module) callback, void Function(JsError e) onError); |
There was a problem hiding this comment.
can we provide a more explicit type than Object for the callback parameter here and below?
There was a problem hiding this comment.
I think we can't. We know that there is a raw JS object there, but as its keys might be any, we can create @JS annotated type for it
|
|
||
| class Module { | ||
| static String roughLibraryKeyDecode(String moduleId, String key) { | ||
| key = key.replaceAll('__', '/'); |
There was a problem hiding this comment.
This is going to be pretty brittle :(. I would at least file an issue and link it in a TODO, as there isn't any other less hacky solution really available right now.
| class Module { | ||
| static String roughLibraryKeyDecode(String moduleId, String key) { | ||
| key = key.replaceAll('__', '/'); | ||
| key = key.replaceAllMapped( |
There was a problem hiding this comment.
nit: you could just chain this off the other replaceAll call
| } | ||
|
|
||
| class Module { | ||
| static String roughLibraryKeyDecode(String moduleId, String key) { |
There was a problem hiding this comment.
lets add a doc comment for this one describing when/how it should be used
| } | ||
| } | ||
|
|
||
| final Map<String, Library> libraries; |
There was a problem hiding this comment.
what are these indexed by? I would either document it or change it to librariesBy*
There was a problem hiding this comment.
will change it in next PR together with roughLibraryKeyDecode
| if (success == false) { | ||
| return false; | ||
| } else if (success == null) { | ||
| result = success; |
There was a problem hiding this comment.
Should this be a return null.? What are the desired semantics here?
There was a problem hiding this comment.
No, we shouldn't. There are two independent reasons to not return null imidietely:
- There might be more hooks with side effects like restoring inner state that we should call even if we already know that we gonna bubble up to parent
- There might be more hooks that returns false and we need to call them to find it out
There are test cases for this scenarios
|
|
||
| bool onChildUpdate(String childId, Module child, Map<String, Object> data) { | ||
| var result = true; | ||
| // TODO(inayd): This is a rought implementation with lots of false positive |
There was a problem hiding this comment.
lets file a bug about this for tracking purposes as well
| if (success == false) { | ||
| return false; | ||
| } else if (success == null) { | ||
| result = success; |
There was a problem hiding this comment.
same question here, should we immediately return null?
| expect(module.onSelfUpdate({}), false); | ||
| }); | ||
|
|
||
| test('onSelfUpdate should run all even if returns null', () { |
There was a problem hiding this comment.
Ah ok this test pretty much answers my question above - seems like reasonable behavior actually.
Please add doc comments to the implementations about the expected behavior though.
* 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)
Fixes #1760
Fixes #1762