Skip to content

Commit

Permalink
[flutter_tools] Lazily inject logger into web devices (#55961)
Browse files Browse the repository at this point in the history
Constructing the WebDevices with the global logger too early will lead to them grabbing the StdoutLogger when running in daemon mode. This prevents IDEs from seeing the correct debug message.
  • Loading branch information
jonahwilliams committed Apr 29, 2020
1 parent 1d0999d commit 72397fd
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 7 deletions.
1 change: 0 additions & 1 deletion packages/flutter_tools/lib/src/device.dart
Expand Up @@ -93,7 +93,6 @@ class DeviceManager {
WebDevices(
featureFlags: featureFlags,
fileSystem: globals.fs,
logger: globals.logger,
platform: globals.platform,
processManager: globals.processManager,
),
Expand Down
4 changes: 3 additions & 1 deletion packages/flutter_tools/lib/src/web/chrome.dart
Expand Up @@ -15,6 +15,7 @@ import '../base/io.dart';
import '../base/logger.dart';
import '../base/os.dart';
import '../convert.dart';
import '../globals.dart' as globals;

/// An environment variable used to override the location of Google Chrome.
const String kChromeEnvironment = 'CHROME_EXECUTABLE';
Expand Down Expand Up @@ -121,7 +122,7 @@ class ChromiumLauncher {
final Platform _platform;
final ProcessManager _processManager;
final OperatingSystemUtils _operatingSystemUtils;
final Logger _logger;
Logger _logger;
final BrowserFinder _browserFinder;
final FileSystemUtils _fileSystemUtils;

Expand Down Expand Up @@ -162,6 +163,7 @@ class ChromiumLauncher {
bool skipCheck = false,
Directory cacheDir,
}) async {
_logger ??= globals.logger;
if (_currentCompleter.isCompleted) {
throwToolExit('Only one instance of chrome can be started.');
}
Expand Down
16 changes: 11 additions & 5 deletions packages/flutter_tools/lib/src/web/web_device.dart
Expand Up @@ -14,6 +14,7 @@ import '../base/os.dart';
import '../build_info.dart';
import '../device.dart';
import '../features.dart';
import '../globals.dart' as globals;
import '../project.dart';
import 'chrome.dart';

Expand All @@ -35,7 +36,7 @@ abstract class ChromiumDevice extends Device {
@required String name,
@required this.chromeLauncher,
@required FileSystem fileSystem,
@required Logger logger,
Logger logger,
}) : _fileSystem = fileSystem,
_logger = logger,
super(
Expand All @@ -48,7 +49,7 @@ abstract class ChromiumDevice extends Device {
final ChromiumLauncher chromeLauncher;

final FileSystem _fileSystem;
final Logger _logger;
Logger _logger;

/// The active chrome instance.
Chromium _chrome;
Expand Down Expand Up @@ -114,6 +115,7 @@ abstract class ChromiumDevice extends Device {
bool prebuiltApplication = false,
bool ipv6 = false,
}) async {
_logger ??= globals.logger;
// See [ResidentWebRunner.run] in flutter_tools/lib/src/resident_web_runner.dart
// for the web initialization and server logic.
final String url = platformArgs['uri'] as String;
Expand Down Expand Up @@ -239,7 +241,10 @@ class MicrosoftEdgeDevice extends ChromiumDevice {
class WebDevices extends PollingDeviceDiscovery {
WebDevices({
@required FileSystem fileSystem,
@required Logger logger,
// TODO(jonahwilliams): the logger is overriden by the daemon command
// to support IDE integration. Accessing the global logger too early
// will grab the old stdout logger.
Logger logger,
@required Platform platform,
@required ProcessManager processManager,
@required FeatureFlags featureFlags,
Expand Down Expand Up @@ -302,7 +307,7 @@ String parseVersionForWindows(String input) {
/// A special device type to allow serving for arbitrary browsers.
class WebServerDevice extends Device {
WebServerDevice({
@required Logger logger,
Logger logger,
}) : _logger = logger,
super(
'web-server',
Expand All @@ -311,7 +316,7 @@ class WebServerDevice extends Device {
ephemeral: false,
);

final Logger _logger;
Logger _logger;

@override
void clearLogs() { }
Expand Down Expand Up @@ -367,6 +372,7 @@ class WebServerDevice extends Device {
bool prebuiltApplication = false,
bool ipv6 = false,
}) async {
_logger ??= globals.logger;
final String url = platformArgs['uri'] as String;
if (debuggingOptions.startPaused) {
_logger.printStatus('Waiting for connection from Dart debug extension at $url', emphasis: true);
Expand Down
32 changes: 32 additions & 0 deletions packages/flutter_tools/test/general.shard/web/devices_test.dart
Expand Up @@ -4,6 +4,7 @@

import 'package:file/memory.dart';
import 'package:flutter_tools/src/base/logger.dart';
import 'package:flutter_tools/src/build_info.dart';
import 'package:flutter_tools/src/device.dart';
import 'package:flutter_tools/src/web/chrome.dart';
import 'package:flutter_tools/src/web/web_device.dart';
Expand All @@ -15,6 +16,37 @@ import '../../src/context.dart';
import '../../src/testbed.dart';

void main() {
testUsingContext('Grabs context logger if no constructor logger is provided', () async {
final WebDevices webDevices = WebDevices(
featureFlags: TestFeatureFlags(isWebEnabled: true),
fileSystem: MemoryFileSystem.test(),
platform: FakePlatform(
operatingSystem: 'linux',
environment: <String, String>{}
),
processManager: FakeProcessManager.any(),
);

final List<Device> devices = await webDevices.pollingGetDevices();
final WebServerDevice serverDevice = devices.firstWhere((Device device) => device is WebServerDevice)
as WebServerDevice;

await serverDevice.startApp(
null,
debuggingOptions: DebuggingOptions.enabled(
BuildInfo.debug,
startPaused: true,
),
platformArgs: <String, String>{
'uri': 'foo',
}
);

// Verify that the injected testLogger is used.
expect(testLogger.statusText, contains(
'Waiting for connection from Dart debug extension at foo'
));
});
testWithoutContext('No web devices listed if feature is disabled', () async {
final WebDevices webDevices = WebDevices(
featureFlags: TestFeatureFlags(isWebEnabled: false),
Expand Down

0 comments on commit 72397fd

Please sign in to comment.