Repository navigation
Revert: [flutter_tools] Intercept prohibited hot reloads in widget preview and trigger hot restart - #193968
Conversation
…eview and trigger hot restart (flutter#193447)" This reverts commit 42d685b.
|
Detected the If you add the The "Merge" button is also unlocked. To bypass presubmits as well as the tree status, press the GitHub "Add to Merge Queue". |
There was a problem hiding this comment.
Code Review
This pull request simplifies the reload logic in the widget preview command by removing the fallback from hot reload to hot restart when a reload is rejected. It removes the handleReload method, simplifies the reload triggering mechanism, and cleans up associated tests and unused properties. Feedback on the changes highlights a compilation error in widget_preview.dart where the leading-dot shorthand .hotRestart is incorrectly used in an equality comparison, and suggests using the fully qualified enum value _PendingReload.hotRestart instead.
…eview and trigger hot restart (flutter#193968) Reverts: [[flutter_tools] Intercept prohibited hot reloads in widget preview and trigger hot restart](flutter#193447) Initiated by: @bkonyi Reason for reverting: conflicting changes with flutter#192831 are causing failures due to concurrent merge Original PR Author: @bkonyi Reviewed By: @srawlins The original PR description is provided below: When hot reload fails due to unsupported changes (such as class hierarchy modifications, global initializer changes, or file additions/deletions) in `flutter widget-preview`, automatically fall back to a hot restart instead of leaving the previewer in a failed reload state. ### Changes - **`ResidentWebRunner`**: Attach `updateFSReport` to the failed `OperationResult` when `_updateDevFS` fails so callers can inspect `UpdateFSReport.hotReloadRejected`. - **`WidgetPreviewStartCommand`**: Introduce `handleReload()` (invoked inside `_triggerReload` for `.hotReload` requests so the fallback remains serialized with other reload and restart requests, and add a `@visibleForTesting` `widgetPreviewApp` setter), which checks if `restart()` returned an `OperationResult` with `UpdateFSReport(hotReloadRejected: true)`. When rejected, log a status message (`kHotReloadRejectedMessage`) and invoke `restart(fullRestart: true)`. Fixes flutter#192697 ## 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. - [x] All existing and new tests are passing. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
…sts that run the real pub The hermetic commands shard had left out nine files and the permeable half entirely: their tests resolve a project of their own with the real pub (the widget_preview tests' scaffold, `flutter create`'s projects, `flutter pub get` on plugins), and pub wants a Flutter SDK -- not just the Dart SDK and engine stamps the general shard's facsimile under bin/cache already had. This lays the rest of pub's view of an SDK in the runfiles tree, and a pub cache of what the projects reach, so no test downloads a package: * `pub_cache(name, roots, dev_roots, without)` (dart/pub.bzl): the hosted packages of a closure, laid as a pub cache (`hosted/pub.dev/<name>-<version>/` plus the `hosted-hashes` sha256 pub checks) from the pinned sources `@pub` already fetches. dart_test's new `pub_cache` attribute symlinks it under the runfiles `.pub-cache` and the launcher exports PUB_CACHE. pub still asks pub.dev which versions exist unless it runs `--offline` (the tests do not), so such a shard has the network; it downloads nothing it finds in the cache (one package -- webview_flutter_web, outside the workspace's lock -- it does fetch). * `bin/cache/flutter.version.json` (bin/internal: `flutter_version_json`): pub's Flutter SDK source reads `flutterVersion` from it and checks it against the packages' `flutter:` constraints (leak_tracker_flutter_testing wants >=3.18); the tool parses `frameworkVersion` as a git tag. Fixed words again: 999.0.0-0.0.pre, the version every constraint allows. * The SDK packages under FLUTTER_ROOT/packages (flutter, flutter_test, flutter_localizations, flutter_driver, integration_test, fuchsia_remote_debug_protocol and a new BUILD for flutter_web_plugins), the workspace's pubspec.yaml and pubspec.lock (`flutter create` reads the SDK's lock), and the engine's sky_engine package under `bin/cache/pkg/sky_engine` (the analyzer resolves dart:ui from it): `assembled_directory(contents, prefix)` in local_engine.bzl is the `install`/`local_engine` assembly as a rule of its own, and `//engine/src/flutter/sky/packages/sky_engine:package` (a converter appendix) lays dart_pkg's directory and the copied libraries as GN's `gen/dart-pkg/sky_engine`. * `<dart-sdk>/version` in the SDK overlay's `dart` filegroup: the analyzer (`AnalysisContextCollection` with `sdkPath`) reads it; six widget_preview tests needed only this. * `@gradle_wrapper` (flutter_sdk_cache.bzl, from bin/internal's pin): `flutter create` copies `bin/cache/artifacts/gradle_wrapper` into an Android project. * `dart_tests(large = [...])`, as flutter_tests has it: preview_detector_test, preview_detector_workspace_test and preview_code_generator_test take minutes on a loaded machine (the first timed out at 300 s). * //bin:BUILD.bazel exports the Windows entrypoint scripts as text (script_test reads them for what they must not do); the Unix ones stay out on purpose -- a test that finds bin/flutter spawns it, and the script bootstraps the tool from the network. integration_test's `sources` gain its Android embedding: the package is a plugin, and the tool reads the main class when a project that depends on it resolves. `//packages/flutter_tools:commands_shard_hermetic` is now the whole directory (67 files, up from 58); `commands_shard_permeable` is new (8 of 11: create_test analyzes, resolves and tests the projects it creates with bin/cache/flutter_tools.snapshot and icon_tree_shaker_test spawns bin/flutter -- the built tool, which a later change can lay in the facsimile from Bazel's own compile; widget_preview_test does not compile at this checkout, flutter#192831 and flutter#193447 crossed and upstream reverted the latter in flutter#193968). Both are in `//:tool_tests`. The README's tests section describes the facsimile. Measured: bazel test //packages/flutter_tools:commands_shard_hermetic \ //packages/flutter_tools:commands_shard_permeable 75 of 75 targets pass; 1,140 cases passed, 6 skipped dart test (FLUTTER_ROOT set, -j 4) over the same 75 files: 1,140 + 6 bazel test //:tool_tests //:dart_tests: 406 of 406 targets pass gn_to_bazel --check clean; converter tests 231, bazel_tools tests 43.
Reverts: [flutter_tools] Intercept prohibited hot reloads in widget preview and trigger hot restart
Initiated by: @bkonyi
Reason for reverting: conflicting changes with #192831 are causing failures due to concurrent merge
Original PR Author: @bkonyi
Reviewed By: @srawlins
The original PR description is provided below:
When hot reload fails due to unsupported changes (such as class hierarchy modifications, global initializer changes, or file additions/deletions) in
flutter widget-preview, automatically fall back to a hot restart instead of leaving the previewer in a failed reload state.Changes
ResidentWebRunner: AttachupdateFSReportto the failedOperationResultwhen_updateDevFSfails so callers can inspectUpdateFSReport.hotReloadRejected.WidgetPreviewStartCommand: IntroducehandleReload()(invoked inside_triggerReloadfor.hotReloadrequests so the fallback remains serialized with other reload and restart requests, and add a@visibleForTestingwidgetPreviewAppsetter), which checks ifrestart()returned anOperationResultwithUpdateFSReport(hotReloadRejected: true). When rejected, log a status message (kHotReloadRejectedMessage) and invokerestart(fullRestart: true).Fixes #192697
Pre-launch Checklist
///).