[flutter_tools] Serve DevTools at app start by kenzieschmoll · Pull Request #73366 · flutter/flutter

kenzieschmoll · 2021-01-05T23:11:20Z

This PR:

Serves DevTools at app run / attach
Adds a link to DevTools to terminal output (right after the existing Observatory link)
Removes the 'v' terminal hotkey, since the DevTools uri is now printed to CLI
Adds a run flag --devtools-server-address so that an existing DevTools server can be passed to flutter_tools (this will be used by VS Code @DanTup)

packages/flutter_tools/lib/src/resident_runner.dart

kenzieschmoll · 2021-01-05T23:19:53Z

packages/flutter_tools/lib/src/run_cold.dart

+        await Future.wait(<Future<void>>[
+          connectToServiceProtocol(),
+          serveDevToolsGracefully(),
+        ]);


attach wasn't being called from flutter run for profile mode like it was for debug mode, so I had to add this to run in addition to attach for the cold runner

What happens when we attach to a running profile mode app?

attach is called in run_cold.dart

jonahwilliams · 2021-01-06T23:17:07Z

packages/flutter_tools/lib/src/devtools_launcher.dart

-        status.cancel();
-        _logger.printError('Error running `pub global activate '
-            'devtools`:\n${_devToolsActivateProcess.stderr}');
+      await http.head('https://pub.dev');


Make sure to include a check for https://flutter.dev/community/china#configuring-flutter-to-use-a-mirror-site , since otherwise this will fail in China.

https://pub.dev won't be accessible though, so one of these will always fail. Instead you should look up if there is a pub url override in the environment variable (the user could have some arbitrary pub mirror configured) and use that if present, falling back to pub.dev

kenzieschmoll · 2021-01-07T22:19:07Z

@DanTup see the latest commit for --devtools-server-address flag

jonahwilliams · 2021-01-07T23:17:45Z

packages/flutter_tools/lib/src/devtools_launcher.dart

-        status.cancel();
-        _logger.printError('Error running `pub global activate '
-            'devtools`:\n${_devToolsActivateProcess.stderr}');
+      await http.head('https://pub.dev');


https://pub.dev won't be accessible though, so one of these will always fail. Instead you should look up if there is a pub url override in the environment variable (the user could have some arbitrary pub mirror configured) and use that if present, falling back to pub.dev

packages/flutter_tools/lib/src/devtools_launcher.dart

packages/flutter_tools/lib/src/persistent_tool_state.dart

jonahwilliams · 2021-01-07T23:20:55Z

packages/flutter_tools/lib/src/resident_runner.dart

+    } else {
+      try {
+        return await _devToolsLauncher.serve(openInBrowser: openInBrowser);
+      } catch (e) { // ignore: avoid_catches_without_on_clauses


Why ignore errors here?

We don't want a failed devtools serve to halt a flutter run / attach process, so we swallow the error to fail gracefully. Do you have another suggestion?

yes but you are catching all errors, including things like TypeError. What exceptions are you expecting here - could the _devToolsLauncher handle that instead?

Friendly ping here - please don't ignore avoid_catches_without_on_clauses

removed try catch from resident_runner and pushed exception swallowing into devtools_launcher

packages/flutter_tools/lib/src/run_cold.dart

packages/flutter_tools/lib/src/run_hot.dart

packages/flutter_tools/lib/src/runner/flutter_command.dart

kenzieschmoll · 2021-01-08T16:31:06Z

Done addressing review comments PTAL

packages/flutter_tools/test/general.shard/devtools_launcher_test.dart

jonahwilliams · 2021-01-08T20:10:11Z

packages/flutter_tools/lib/src/resident_runner.dart

+
+  Future<void> serveDevToolsGracefully({
+    Uri devToolsServerAddress,
+    bool openInBrowser = false,


What is the functionality of openInBrowser now? Can we let the user click on the link themselves so they get their preferred browser/existing instance

Good question. I'm following up with @helin24 on this to see what we do from IntelliJ.

Added a TODO to remove this in a cleanup CL, as it will require a g3fix, and I'd like to get this landed sooner than later to start working on iterative CLs. AFAIK the 'v' hotkey was the only place using this functionality openInBrowser: true, so removing the param should be safe

Sounds reasonable!

jonahwilliams · 2021-01-08T20:10:42Z

packages/flutter_tools/lib/src/resident_runner.dart

+    } else {
+      try {
+        return await _devToolsLauncher.serve(openInBrowser: openInBrowser);
+      } catch (e) { // ignore: avoid_catches_without_on_clauses


Friendly ping here - please don't ignore avoid_catches_without_on_clauses

packages/flutter_tools/lib/src/devtools_launcher.dart

Co-authored-by: Jonah Williams <[email protected]>

jonahwilliams

LGTM

DanTup · 2021-01-11T12:30:29Z

@kenzieschmoll are there any existing deep-links generated by Flutter that I can use to test --devtools-server-address when spawned by VS Code?

kenzieschmoll · 2021-01-11T16:17:50Z

@DanTup No, not yet. But if you pass a server address with a port number different than the default 9100, you should be able to verify that devtools is running on the port you specified, like 9105 for example.

DanTup · 2021-01-11T17:17:42Z

@kenzieschmoll that seems to work great. I was mainly curious about how the links would appear in VS Code - for example wondering if we could route them to the internal embedded view in future if that's enabled. I'll come back to that once there are some links. Thanks!

This reverts commit 1cb0a24.

…#73896)" This reverts commit 388dcd2.

…#73896)" (#73903) This reverts commit 388dcd2.

[flutter_tools] Serve DevTools at app start

76086b7

flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 5, 2021

google-cla bot added the cla: yes label Jan 5, 2021

kenzieschmoll commented Jan 5, 2021

View reviewed changes

packages/flutter_tools/lib/src/resident_runner.dart Outdated Show resolved Hide resolved

use getter instead of method

fbbd2e9

kenzieschmoll commented Jan 5, 2021

View reviewed changes

Add and fix tests

7d87866

kenzieschmoll marked this pull request as ready for review January 6, 2021 16:22

Ensure the offline use case is WAI

7a0c8c7

jonahwilliams reviewed Jan 6, 2021

View reviewed changes

kenzieschmoll and others added 3 commits January 6, 2021 15:32

Limit the number of times we call pub global activate to 2 per day

ca7700e

Add check to china hosted site

0f9c3c2

Merge branch 'master' into devtools-launch

3d25c5d

jonahwilliams self-requested a review January 7, 2021 21:48

Add --devtools-server-address flag

4aa9d55

remove unused import

6d6aa92

jonahwilliams reviewed Jan 7, 2021

View reviewed changes

kenzieschmoll added 3 commits January 7, 2021 15:41

add timeout to devtools.launch

9fcc13f

Review comments and add test.

15ebd4a

Check platform environment variables for PUB_HOSTED_URL

765ed3e

jonahwilliams reviewed Jan 8, 2021

View reviewed changes

review comments.

512e64e

jonahwilliams reviewed Jan 8, 2021

View reviewed changes

packages/flutter_tools/lib/src/devtools_launcher.dart Outdated Show resolved Hide resolved

kenzieschmoll and others added 2 commits January 8, 2021 12:54

move todo to base class

e6d4d1d

Update packages/flutter_tools/lib/src/devtools_launcher.dart

0007269

Co-authored-by: Jonah Williams <[email protected]>

jonahwilliams approved these changes Jan 8, 2021

View reviewed changes

kenzieschmoll added 2 commits January 8, 2021 14:02

add files missing from earlier commit

f6455e1

Set devToolsUri to null in close

4fde2ff

Merge branch 'master' of github.com:flutter/flutter into devtools-launch

3907447

kenzieschmoll added 2 commits January 12, 2021 16:02

remove openInBrowser param

d7d319a

fix test

68ea030

kenzieschmoll merged commit 1cb0a24 into flutter:master Jan 13, 2021

kenzieschmoll deleted the devtools-launch branch January 13, 2021 05:39

jmagman added a commit that referenced this pull request Jan 13, 2021

Revert "[flutter_tools] Serve DevTools at app start (#73366)"

e8b78cc

This reverts commit 1cb0a24.

jmagman mentioned this pull request Jan 13, 2021

Revert "[flutter_tools] Serve DevTools at app start" #73896

Merged

jmagman added a commit that referenced this pull request Jan 13, 2021

Revert "[flutter_tools] Serve DevTools at app start (#73366)" (#73896)

388dcd2

This reverts commit 1cb0a24.

jmagman added a commit that referenced this pull request Jan 13, 2021

Revert "Revert "[flutter_tools] Serve DevTools at app start (#73366)" (…

437a309

…#73896)" This reverts commit 388dcd2.

jmagman mentioned this pull request Jan 13, 2021

Revert "Revert "[flutter_tools] Serve DevTools at app start"" #73903

Merged

jmagman added a commit that referenced this pull request Jan 13, 2021

Revert "Revert "[flutter_tools] Serve DevTools at app start (#73366)" (…

912c3ab

…#73896)" (#73903) This reverts commit 388dcd2.

kenzieschmoll mentioned this pull request Jan 19, 2021

flutter_immediately_exit_test integration test failures caused by devtools serving, Expected: contains 'Error connecting to the service protocol:', Actual: 'Unhandled exception: SocketException' #74052

Closed

kenzieschmoll mentioned this pull request Feb 2, 2021

Always show Starting DevTools flutter/flutter-intellij#5234

Closed

jmagman mentioned this pull request Feb 9, 2021

Consolidate ascii spinner stop in devtools activation #75679

Merged

kenzieschmoll mentioned this pull request Mar 11, 2021

Launch DevTools at app start for flutter web apps #77975

Closed

kenzieschmoll mentioned this pull request May 10, 2021

[flutter_tools] Add back the 'v' hotkey for launching DevTools #82206

Closed

Conversation

kenzieschmoll commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenzieschmoll commented Jan 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kenzieschmoll commented Jan 8, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

DanTup commented Jan 11, 2021

Uh oh!

kenzieschmoll commented Jan 11, 2021

Uh oh!

DanTup commented Jan 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kenzieschmoll commented Jan 5, 2021 •

edited

Loading