Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Pass hot reload/hot restart callbacks and reload sources path to StrategyProviders instead #2584

Open
srujzs opened this issue Jan 30, 2025 · 2 comments
Open

Pass hot reload/hot restart callbacks and reload sources path to StrategyProviders instead #2584

srujzs opened this issue Jan 30, 2025 · 2 comments
Assignees
@srujzs
Labels
package:dwds

Comments

@srujzs
Copy link
Contributor

srujzs commented Jan 30, 2025 •
edited
Loading

Currently, with hot restart, we call a developer extension method to disassemble the Flutter engine after we've successfully hot restarted the application. This allows the Flutter engine to restart state.

As we add hot reload, we're doing something similar: reassembling the Flutter engine so that it renders a new frame after a hot reload.

Similarly, we assume there exists a path in the global window that the injected client can request to find the paths that have changed for a hot reload.

This would also help analytics determine the cost of reload vs reassemble.

These are all implicit assumptions that are tied to Flutter tools. We should avoid this and instead have the providers take several options:

  • One for developer extensions (including args) that need to be called after a hot restart (note that DWDS must do this as it is able to evaluate JS expressions, where Flutter tools does not).
  • One for developer extensions (including args) that need to be called after a hot reload, like reassemble.
  • One for the reload sources path that DWDS fetches from.
@srujzs srujzs self-assigned this Jan 30, 2025
@srujzs srujzs mentioned this issue Jan 30, 2025
Merged
@srujzs
Copy link
Contributor Author

srujzs commented Jan 31, 2025 •
edited
Loading

Actually, the vm service has a hook to invoke a service extension (specifically reassemble). Perhaps we should use that instead and use more of the run_hot.dart implementation in general.

srujzs added a commit to srujzs/webdev that referenced this issue Jan 31, 2025
@srujzs
[dwds] Remove reassemble extension invocation for restarter
35225b0 
dart-lang#2584

This can and should be done in Flutter tools.
@srujzs srujzs mentioned this issue Jan 31, 2025
Closed
srujzs added a commit to srujzs/flutter that referenced this issue Jan 31, 2025
@srujzs
Implement hot reload for the web
e2a5491 
- Updates restart/reload code to accept a resetCompiler boolean
to disambiguate between whether this is a full restart and whether
to reset the resident compiler.
- Adds code to call reloadSources in DWDS and handle the response
(including any errors).
- Adds code to invoke reassemble.
- Adds code to emit a script that DWDS can later consume that contains
the changed sources and their associated libraries. This is used to
hot reload. The bootstrapper puts this in the global window. DWDS should
be updated to accept it in the provider itself. See
dart-lang/webdev#2584.
- Adds code to parse module metadata from the frontend server. This is
identical to the implementation in DWDS % addressing type-related lints.
- Adds tests that run the existing hot reload tests but with web. Some
modifications are mode, including waiting for Flutter runs to finish
executing, and skipping a test that's not possible on the web.
@srujzs srujzs mentioned this issue Jan 31, 2025
Merged
9 tasks
github-merge-queue bot pushed a commit to flutter/flutter that referenced this issue Feb 3, 2025
@srujzs
Implement hot reload using the DDC library bundle format (#162498)
8e2a6fc 
dart-lang/webdev#2516

- Updates restart/reload code to accept a resetCompiler boolean to
disambiguate between whether this is a full restart and whether to reset
the resident compiler.
- Adds code to call reloadSources in DWDS and handle the response
(including any errors).
- Adds code to invoke reassemble.
- Adds code to emit a script that DWDS can later consume that contains
the changed sources and their associated libraries. This is used to hot
reload. The bootstrapper puts this in the global window. DWDS should be
updated to accept it in the provider itself. See
dart-lang/webdev#2584.
- Adds code to parse module metadata from the frontend server. This is
identical to the implementation in DWDS % addressing type-related lints.
- Adds tests that run the existing hot reload tests but with web. Some
modifications are mode, including waiting for Flutter runs to finish
executing, and skipping a test that's not possible on the web.

Needs DWDS 24.3.4 to be published first and used before we can land.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [ ] All existing and new tests are passing.
@srujzs
Copy link
Contributor Author

srujzs commented Feb 14, 2025 •
edited
Loading

A consideration on moving reassemble to Flutter tools instead of DWDS: it may come across a race condition where files are reloaded and execution continues, but the UI hasn't been updated yet.

Since reassemble is asynchronous, this is unavoidable. We can't execute it in the debugger while the program is paused because it is asynchronous, and waiting for a Promise outside of the debugger (and therefore, not paused) may continue executing user code first.

@srujzs srujzs changed the title Pass developer extension configurations and reload sources path to FrontendServerDdcLibraryBundleStrategyProvider and FrontendServerRequireStrategyProvider instead Pass developer extension configurations and reload sources path to StrategyProviders instead Feb 20, 2025
@srujzs srujzs changed the title Pass developer extension configurations and reload sources path to StrategyProviders instead Pass hot reload/hot restart callbacks and reload sources path to StrategyProviders instead Feb 20, 2025
srujzs added a commit to srujzs/webdev that referenced this issue Mar 3, 2025
@srujzs
Add hotReloadSourcesUri parameter to FrontendServerDdcLibraryBundlePr…
13310e8 
…ovider and publish DWDS 24.3.6

dart-lang#2584

During hot reload, a JSON file that contains the sources and libraries
that need to be loaded and reloaded is expected. Before, Flutter tools
was adding this script to the global window. It should instead be
directly passed to the provider to be used later.
@srujzs srujzs mentioned this issue Mar 3, 2025
Merged
srujzs added a commit that referenced this issue Mar 4, 2025
@srujzs
Add hotReloadSourcesUri parameter to FrontendServerDdcLibraryBundlePr…
8a8eaf1 
…ovider and publish DWDS 24.3.6 (#2594)

#2584

During hot reload, a JSON file that contains the sources and libraries
that need to be loaded and reloaded is expected. Before, Flutter tools
was adding this script to the global window. It should instead be
directly passed to the provider to be used later.
@srujzs srujzs mentioned this issue Mar 4, 2025
Merged
9 tasks
github-merge-queue bot pushed a commit to flutter/flutter that referenced this issue Mar 5, 2025
@srujzs
Use dwds 24.3.6 and pass uri for the reload scripts path to FrontendS…
86e4c12 
…erverDdcLibraryBundleProvider (#164582)

The path to reload scripts JSON was being added to the global window
before. It should instead be passed to the provider on setup. In order
to do this, the baseUri calculation is moved into the WebAssetServer.
This is safe because Flutter tools was calculating it immediately
after creating the server anyways.

dart-lang/webdev#2584

Existing hot reload tests should test this behavior.

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant documentation (doc comments with `///`).
- [ ] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [ ] All existing and new tests are passing.
srujzs added a commit to srujzs/webdev that referenced this issue Mar 6, 2025
@srujzs
[dwds] callServiceExtension should check all extensions and return kM…
f5587a4 
…ethodNotFound when extension not found and remove reassemble from reload

dart-lang#2584

Currently, callServiceExtension checks the extensionRpcs field in the VM
service. This list is populated when DWDS starts and checks the extensions
currently registered in the runtime. This list is prone to becoming stale,
however, as services may be registered or removed through the runtime
(via dart:developer) later. This is true for ext.flutter.reassemble, where DWDS
can not find the service because we computed the list too early.

Instead, we should seek to always check the runtime for the current registered
extensions and then update this list whenever we do. It’s still prone to becoming
stale, but it is less stale. Ideally, we would override extensionRpcs and fetch the
current registered extensions, but that isn’t possible since it requires awaiting a
future. We also can’t ignore this field altogether because it looks like clients, like
Dart DevTools, uses this field.

callServiceExtension should also return kMethodNotFound instead of its current
JS evaluation error when an extension is not present.

Lastly, the reassemble call is removed from reload as it can now be safely called in
Flutter tools instead.
@srujzs srujzs mentioned this issue Mar 6, 2025
Open
srujzs added a commit to srujzs/webdev that referenced this issue Mar 7, 2025
@srujzs
Remove reassemble invocation and publish 24.3.7
9fff37e 
dart-lang#2584

This can and should be done in Flutter tools.
@srujzs srujzs mentioned this issue Mar 7, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@srujzs srujzs

Labels
package:dwds
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@srujzs