Fix: --no-hot flag should trigger hot restart instead of being ignored#185245
Fix: --no-hot flag should trigger hot restart instead of being ignored#185245Jah-yee wants to merge 2 commits intoflutter:masterfrom
Conversation
In platform_isolate_manager.cc, fix spelling errors in comment: - 'occured' → 'occurred' (missing 'r') - 'aquire' → 'acquire' (missing 'c')
|
It looks like this pull request may not have tests. Please make sure to add tests or get an explicit test exemption before merging. If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix? Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing. If you believe this PR qualifies for a test exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
There was a problem hiding this comment.
Code Review
This pull request propagates the hotMode flag through the Flutter tool's runner and command infrastructure, including the daemon, run command, and web runner. It also updates the terminal handler to perform a hot restart when hot reload is unavailable, such as when the --no-hot flag is used. Feedback identifies a compilation error in ResidentWebRunner due to redundant super parameter usage and suggests refactoring the restart logic in TerminalHandler to reduce duplication.
| super( | ||
| <FlutterDevice>[device], | ||
| target: target ?? fileSystem.path.join('lib', 'main.dart'), | ||
| hotMode: hotMode, |
There was a problem hiding this comment.
This will cause a compilation error. In Dart, a super parameter (like super.hotMode on line 105) cannot be used in the initializer list of the constructor. Since it's already being passed to the super constructor via the super-parameter syntax, it should be removed from the explicit super() call.
| if (!residentRunner.canHotReload) { | ||
| return false; | ||
| // If hot reload is disabled (e.g., --no-hot flag), still allow | ||
| // hot restart via 'r' key since that's the expected behavior. | ||
| if (!residentRunner.supportsRestart) { | ||
| return false; | ||
| } | ||
| final OperationResult result = await residentRunner.restart(fullRestart: true); | ||
| if (result.fatal) { | ||
| throwToolExit(result.message); | ||
| } | ||
| if (!result.isOk) { | ||
| _logger.printStatus('Try again after fixing the above error(s).', emphasis: true); | ||
| } | ||
| return true; | ||
| } | ||
| final OperationResult result = await residentRunner.restart(); |
There was a problem hiding this comment.
The logic for handling the restart result is duplicated. This can be simplified by determining whether a full restart is needed and then performing a single call to restart with the appropriate flag.
final bool fullRestart = !residentRunner.canHotReload;
if (fullRestart && !residentRunner.supportsRestart) {
return false;
}
// If hot reload is disabled, 'r' performs a hot restart.
final OperationResult result = await residentRunner.restart(fullRestart: fullRestart);
Good day,
This PR fixes issue #179448 - the
--no-hotflag on web-server is not working as expected.When
--no-hotis passed, pressing 'r' should perform a hot restart (full reload), not attempt a hot reload which doesn't work in that mode.Changes:
Thank you for your attention. If there are any issues or suggestions, please leave a comment and I will address them promptly.
Warmly,
RoomWithOutRoof