Switch flutter_tools to use frontend_server for web compilation by jonahwilliams · Pull Request #50365 · flutter/flutter

jonahwilliams · 2020-02-07T21:43:29Z

Description

Updates the implementation of the web runner to use the frontend server (previously, experimental incremental compiler config). This should improve compile times, as well as reduce some of the occurrences of crashes due to us embedding build_runner incorrectly.

Fixes #41951

Closes

Fixes #49838
Fixes #49479
Fixes #49002
Fixes #48481
Fixes #47115
Fixes #46877
Fixes #44698
Fixes #44552
Fixes #43639
Fixes #41544
Fixes #40481
Fixes #38797
Fixes #45798

Might help

#49103
#47732
#44459

jonahwilliams · 2020-02-08T00:50:32Z

Sorry @zanderso

zanderso · 2020-02-10T16:07:48Z

packages/flutter_tools/lib/src/build_runner/build_runner_compilation_delegate.dart

@@ -0,0 +1,249 @@
+// Copyright 2014 The Flutter Authors. All rights reserved.


Can't think of one, but maybe this file's name should include 'web' somewhere.

zanderso · 2020-02-10T16:08:24Z

packages/flutter_tools/lib/src/build_runner/build_runner_compilation_delegate.dart

+import 'dart:async';
+
+import 'package:build_daemon/constants.dart' as daemon;
+import 'package:build_daemon/client.dart';


Alphabetize

zanderso · 2020-02-10T16:28:42Z

packages/flutter_tools/lib/src/build_runner/build_runner_compilation_delegate.dart

+      .createSync();
+    final FlutterProject flutterProject = FlutterProject.fromDirectory(projectDirectory);
+    final bool hasWebPlugins = findPlugins(flutterProject)
+        .any((Plugin p) => p.platforms.containsKey(WebPlugin.kConfigKey));


nit: 2 space indent on continued line

zanderso · 2020-02-10T16:34:00Z

packages/flutter_tools/lib/src/build_runner/build_runner_compilation_delegate.dart

+    await for (final BuildResults results in client.buildResults) {
+      final BuildResult result = results.results.firstWhere((BuildResult result) {
+        return result.target == 'web';
+      });


If firstWhere doesn't have an orElse:, it will throw a StateError when it can't find something that matches. Since this code is in an await for, I'd prefer it failing in a more controlled way.

zanderso · 2020-02-10T16:34:57Z

packages/flutter_tools/lib/src/build_runner/build_runner_compilation_delegate.dart

+        break;
+      }
+    }
+    if (success && testOutputDir != null) {


nit: Flip the sense and unindent the larger part.

Alternately, split the test copying code out into a helper function.

zanderso · 2020-02-10T17:15:30Z

packages/flutter_tools/lib/src/web/devfs_web.dart

+      final DebugConnection debugConnection = useDebugExtension
+        ? await (_cachedExtensionFuture ??= dwds.extensionDebugConnections.stream.first)
+        : await dwds.debugConnection(appConnection);
+      if (!firstConnection.isCompleted) {


nit: think positive

zanderso · 2020-02-10T17:16:39Z

packages/flutter_tools/lib/src/web/devfs_web.dart

+  /// Only calls [AppConnection.runMain] on the subsequent connections.
+  Future<ConnectionResult> connect(bool useDebugExtension) {
+    final Completer<ConnectionResult> firstConnection = Completer<ConnectionResult>();
+    _connectedApps = dwds.connectedApps.listen((AppConnection appConnection) async {


listen should generally have an onError

zanderso · 2020-02-10T17:18:38Z

packages/flutter_tools/lib/src/web/devfs_web.dart

+  Future<ConnectionResult> connect(bool useDebugExtension) {
+    final Completer<ConnectionResult> firstConnection = Completer<ConnectionResult>();
+    _connectedApps = dwds.connectedApps.listen((AppConnection appConnection) async {
+      final DebugConnection debugConnection = useDebugExtension


If this listen callback throws an exception it will be an unhandled exception.

Updated to forward the error through the completer

onError handles errors on the stream, but won't run if the listen callback throws.

zanderso · 2020-02-10T17:20:07Z

packages/flutter_tools/lib/src/web/devfs_web.dart

+      final String entrypoint = globals.fs.path.basename(mainPath);
      webAssetServer.writeFile('/manifest.json', '{"info":"manifest not generated in run mode."}');
      webAssetServer.writeFile('/flutter_service_worker.js', '// Service worker not loaded in run mode.');
      webAssetServer.writeFile(


2 space indent, paren style.

zanderso · 2020-02-10T17:20:54Z

packages/flutter_tools/lib/src/web/devfs_web.dart

  }
  return path;
 }
+


extra newline

yjbanov · 2020-02-08T04:58:57Z

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

+    // when(mockWebFs.recompile()).thenAnswer((Invocation _) {
+    //   return Future<bool>.value(false);
+    // });
+    // when(mockWebFs.uri).thenReturn('http://localhost:8765/app/');


Will the above commented out code be useful in the future?

yjbanov · 2020-02-08T05:04:35Z

packages/flutter_tools/lib/src/build_runner/build_runner_compilation_delegate.dart

+      });
+      if (result.status == BuildStatus.failed) {
+        success = false;
+        break;


Why is break needed? Does the build not stop after this? If so, should it be explicitly stopped?

In this particular case, we only want the first success

yjbanov · 2020-02-10T17:15:16Z

packages/flutter_tools/lib/src/build_runner/resident_web_runner.dart

-      );
-    }
-    return _DwdsResidentWebRunner(
+    return _ExperimentalResidentWebRunner(


Remove "Experimental" from the name?

yjbanov · 2020-02-10T17:21:48Z

packages/flutter_tools/lib/src/build_runner/resident_web_runner.dart

    try {
-      if (fullRestart || !debuggingOptions.buildInfo.isDebug) {
+      if (!deviceIsDebuggable) {
+        globals.printStatus('Recompile complete. Page requires refresh.');


nit: it's unclear what the use is supposed to do when the page requires a refresh. Perhaps tell them what to do? E.g. "Please refresh the page using browser's reload button"

Lets leave this for now since it matches the current behavior

yjbanov · 2020-02-10T17:22:00Z

packages/flutter_tools/lib/src/build_runner/resident_web_runner.dart

          'ignoreCache': !debuggingOptions.buildInfo.isDebug,
        });
-      } else {
+      }  else {


yjbanov · 2020-02-10T18:39:27Z

packages/flutter_tools/lib/src/web/devfs_web.dart

+  File _resolveDartFile(String path) {
+    // If this is a dart file, it must be on the local file system and is
+    // likely coming from a source map request. Attempt to look in the
+    // local filesystem for it, and return a 404 if it is not found. The tool


Should the comment about 404 move somewhere else? This function cannot return 404.

Updated comment

yjbanov · 2020-02-10T18:41:36Z

packages/flutter_tools/lib/src/web/devfs_web.dart

+      if (!firstConnection.isCompleted) {
+        firstConnection.complete(ConnectionResult(appConnection, debugConnection));
+      } else {
+        appConnection.runMain();


nit: it's a bit strange to see a method called connect kick off the main.

Its how we do --start-paused on the web. If we didn't want to pause we need to tell the "isolate" to start

yjbanov · 2020-02-10T18:44:45Z

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

-    expect(didSkipDwds, true);
-  }));
+  //   expect(didSkipDwds, true);
+  // }));


yjbanov · 2020-02-10T18:44:52Z

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

+  //   verify(mockWebFs.connect(true)).called(1);
+  //   // And ensure the debug services was started.
+  //   expect(testLogger.statusText, contains('Debug service listening on'));
+  // }));


yjbanov · 2020-02-10T18:45:49Z

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


    await result;
-    verify(mockWebFs.stop()).called(1);
+    // verify(mockWebFs.stop()).called(1);


Is this still needed?

jonahwilliams · 2020-02-10T21:01:08Z

This currently doesn't work on windows, investigating...

jonahwilliams · 2020-02-11T01:56:14Z

Update to fix on windows, we no longer need to chop off C:/ and friends from the source maps.

jonahwilliams · 2020-02-11T06:17:54Z

Ahh and the package update broke a bunch of stuff ....great

zanderso

lgtm w/ small comment

jonahwilliams · 2020-02-11T20:39:51Z

I added another layer of try-catch

jonahwilliams added 3 commits January 31, 2020 12:49

[flutter_tools] dwds landing work

2e9f5f0

Merge branch 'master' of github.com:flutter/flutter into land_dwds

3ef982c

make it work, but for real

8d45883

fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Feb 7, 2020

googlebot added the cla: yes label Feb 7, 2020

jonahwilliams added 4 commits February 7, 2020 13:59

make sure flutter test still works

f7ca797

more test fixes

4f1e4df

fix more tests

7c904f1

compute versions

45ca805

jonahwilliams marked this pull request as ready for review February 7, 2020 23:50

fix for run tests

f4d4057

jonahwilliams removed the work in progress; do not review label Feb 8, 2020

jonahwilliams requested a review from yjbanov February 8, 2020 00:16

jonahwilliams requested a review from zanderso February 8, 2020 00:50

Jonah Williams added 5 commits February 7, 2020 18:15

Update build_runner_compilation_delegate.dart

b638b08

Update resident_web_runner.dart

77f957c

Update devfs_web.dart

d0a0d8a

update tests for dwds and shelf

08d2b9b

fix more tests

64a99ca

jonahwilliams mentioned this pull request Feb 8, 2020

Can't run flutter run -d chrome test/foo_test.dart #49838

Closed

jonahwilliams added 3 commits February 8, 2020 10:13

fix more tests

475cd02

try to fix dwds test

5c4e2d9

Update devfs_web_test.dart

c177e1e

zanderso reviewed Feb 10, 2020

View reviewed changes

jonahwilliams added 2 commits February 10, 2020 10:13

reduce diff size

6f4b9fd

address review comments from zanderso

89f42c8

yjbanov reviewed Feb 10, 2020

View reviewed changes

jonahwilliams added 2 commits February 10, 2020 10:53

reverse file checking

62c41b5

address yjbanov reviews

0457fa5

jonahwilliams added 3 commits February 10, 2020 13:12

add handling for failure of initial compile

18605a0

Merge branch 'master' into land_dwds

f1d6a8d

fix windows and update pubspecs

a754206

jonahwilliams added 3 commits February 10, 2020 22:25

fix type error in coverage collector

8c03243

Merge branch 'master' of github.com:flutter/flutter into land_dwds

21bf6e9

fix test for remove driver letter logic and pin video player

e6c9613

jonahwilliams requested review from yjbanov and zanderso February 11, 2020 18:22

zanderso approved these changes Feb 11, 2020

View reviewed changes

Update devfs_web.dart

55d2a58

jonahwilliams mentioned this pull request Feb 11, 2020

Capture analytics event from failure to initialize incremental compiler in build runner #43914

Closed

jonahwilliams added the waiting for tree to go green label Feb 11, 2020

yjbanov approved these changes Feb 11, 2020

View reviewed changes

jonahwilliams merged commit 18f38cd into flutter:master Feb 11, 2020

jonahwilliams deleted the land_dwds branch February 11, 2020 23:43

jonahwilliams mentioned this pull request Feb 12, 2020

[web]: flutter build web works but run -d chrome fails. #50640

Closed

yjbanov mentioned this pull request Feb 13, 2020

Stack traces in debug mode point to JavaScript instead of Dart #50753

Closed

This was referenced Feb 15, 2020

Stuck at Building application for the web (Error compiling dartdevc module:jabalstore_mobile) #40250

Closed

flutter run -d chrome: Unable to find modules for some sources #50975

Closed

Flutter run web application failed #40937

Closed

CareF mentioned this pull request Aug 17, 2020

update dev/integration_tests/flutter_gallery/android/settings.gradle #63200

Closed

13 tasks

github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021

		@@ -0,0 +1,249 @@
		// Copyright 2014 The Flutter Authors. All rights reserved.

Conversation

jonahwilliams commented Feb 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Closes

Might help

Uh oh!

jonahwilliams commented Feb 8, 2020

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!

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!

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!

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!

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!

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

jonahwilliams commented Feb 7, 2020 •

edited

Loading