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.

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

[flutter_tools] add needsFullRestart flag on hot runner #104562

Merged
merged 9 commits into from
Jun 3, 2022

Conversation

Jasguerrero
Copy link
Contributor

@Jasguerrero Jasguerrero commented May 24, 2022

fixes: #92000

Pre-launch Checklist

  • I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
  • I read the [Tree Hygiene] wiki page, which explains my responsibilities.
  • I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement].
  • I signed the [CLA].
  • I listed at least one issue that this PR fixes in the description above.
  • 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].
  • All existing and new tests are passing.

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label May 24, 2022
@Jasguerrero Jasguerrero changed the title add needsFullRestart flag [flutter_tools] add needsFullRestart flag on hot runner May 26, 2022
@Jasguerrero Jasguerrero marked this pull request as ready for review May 26, 2022 17:03
@@ -228,6 +228,7 @@ class HotRunner extends ResidentRunner {
Completer<void> appStartedCompleter,
bool allowExistingDdsInstance = false,
bool enableDevTools = false,
bool needsFullRestart = true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of adding it here, I would add it to the ResidentRunner base class and document it. While its technically allowed to add new named parameters to overriden methods on subclasses, I think you should generally avoid that practice. It occasionally leads to surprises when a developer can't autocomplete a parameter due to the static type of a variable.

@@ -1183,6 +1183,7 @@ abstract class ResidentRunner extends ResidentHandlers {
Completer<void> appStartedCompleter,
bool allowExistingDdsInstance = false,
bool enableDevTools = false,
bool needsFullRestart = true,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a doc comment on what needsFullRestart is for?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doc comments should go on the method itself. Look at the flutter framework for examples of how parameters are documented. Usually something like:

/// One sentence summary about what foo does
///
/// [bar] is blah blah.
void foo(String bar);
```

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following this particular example which apparently to have more than one line is acceptable. About going in the method agree I should move this up. https://dart.dev/guides/language/effective-dart/documentation#avoid-redundancy-with-the-surrounding-context

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thats fine, but it needs to go on the method itself and not the parameter

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with nit

@@ -1178,11 +1178,14 @@ abstract class ResidentRunner extends ResidentHandlers {
String route,
});

/// The first time we do a run hot a full restart is not needed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like:

/// Connect to a flutter application.
///
/// [needsFullRestart] defaults to `true`, and controls if the frontend server should
/// compile a full dill. This should be set to `false` if this is called in [ResidentRunner.run], since that method already perfoms an initial compilation.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

@fluttergithubbot fluttergithubbot merged commit 889a15e into flutter:master Jun 3, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 3, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Jun 3, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 4, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 5, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 6, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jun 7, 2022
@Jasguerrero Jasguerrero deleted the run_compiles_twice branch June 13, 2022 18:10
camsim99 pushed a commit to camsim99/flutter that referenced this pull request Aug 10, 2022
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

flutter run compiles twice
3 participants