Remove dart:io/dart:isolate from transitive closure of web applications #38773

jonahwilliams · 2019-08-18T21:00:13Z

Description

I'm pulling the thread on #34858. I've discovered that the hack I added to psuedo-support dart:io has lead to debug and release web builds resolving config specific imports differently. The fix is to remove dart:io usage from framework, which will allow the incremental web compiler to correctly select the right config import.

Background

Flutter for web and Flutter applications have access to different sets of dart:* libraries. dart:io and dart:isolate are not available* for the web and dart:html is not available for the vm. One tool we have to write code that works in both environments is config specific imports. These allow us to resolve import uri's based on the set of currently supported dart libraries. For example:

import 'a.dart'
  if (dart.library.bar) 'b.dart'
  if (dart.library.foo) 'c.dart';

In the above code, the analyzer will only see a.dart. When the code is built for a platform the CFE will check each condition in order to see if the library is supported. If none are, then the default import is selected. A default import is always required.

Supported usually means that there is no "supported:false" pair in the libraries.json for the particular platform. In the case of dartdevc/k the incremental web compiler, package:build uses the DartPlatform type instead, which lists all supported libraries.

Since the incremental compiler cannot compile unsupported libraries, I added """support""" for dart:io and dart:isolate to the platform we use. At the time I was not aware this would alter the config specific imports. Furthermore, since dart2js, the release compiler, only uses the libraries.json this leads to disagreements between the two. Specifically:

import 'a.dart'
  if (dart.library.html) 'b.dart'

The above would return b.dart in both dartdevc/k and dart2js

import 'a.dart'
  if (dart.library.io) 'b.dart'
  if (dart.library.html) 'c.dart'

The above would return 'b.dart' in dartdevc/k and 'c.dart' in dart2js.

Unfortunately a common pattern used by package:http is to have the first library be a "stub", while each real implementation is guarded below.

Overview

I think this can be done in a mostly non-breaking way. We have some usage of Platform.isFoo that could really be defaultTargetPlatform, and Platform.resolvedExecutable doesn't actually print useful information in release applications, so that can go.

There are a few places like FileImage where the type is unfortunately part of the public API. One solution would be to change the type to Object/dynamic, and then have the runtime implementation perform the type check. Another would be to define our own stub and substitute that using more config specific imports.

Then of course, there are breaking changes to fix it, like making FileImage take a String path instead.

Some specific changes here:

Replace Platform.isFoo with defaultTargetPlatform. This may not be necessary in the future due to Move Platform (or similar) to dart:core dart-lang/sdk#35969
Remove usage of platform.resolveExecutable. This doesn't return meaningful information in release mode.
Place NetworkAssetBundle behind a config specific import (like I did with network image)
Place FileImage behind a config specific import.

Related Issues

fluttergithubbot · 2019-08-18T21:00:14Z

It looks like this pull request may not have tests. Please make sure to add tests before merging. While there are exceptions to this rule, if this patch modifies code it is probably not an exception.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

codecov · 2019-08-19T18:35:08Z

Codecov Report

Merging #38773 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #38773      +/-   ##
==========================================
- Coverage   56.53%   56.41%   -0.12%     
==========================================
  Files         195      195              
  Lines       18422    18422              
==========================================
- Hits        10414    10392      -22     
- Misses       8008     8030      +22

Flag	Coverage Δ
#flutter_tool	`56.41% <0%> (-0.12%)`	⬇️

Impacted Files	Coverage Δ
...ges/flutter_tools/lib/src/build_runner/web_fs.dart	`26.22% <ø> (-0.22%)`	⬇️
.../flutter_tools/lib/src/android/android_device.dart	`34% <ø> (ø)`	⬆️
...utter_tools/lib/src/build_runner/build_script.dart	`0% <0%> (ø)`	⬆️
...er_tools/lib/src/build_system/targets/windows.dart	`6.25% <0%> (-87.5%)`	⬇️
packages/flutter_tools/lib/src/compile.dart	`64.21% <0%> (-12.71%)`	⬇️
packages/flutter_tools/lib/src/version.dart	`90.9% <0%> (-1.92%)`	⬇️
packages/flutter_tools/lib/src/cache.dart	`43.02% <0%> (-0.73%)`	⬇️
packages/flutter_tools/lib/src/base/logger.dart	`82.06% <0%> (-0.39%)`	⬇️
packages/flutter_tools/lib/src/artifacts.dart	`68.75% <0%> (-0.35%)`	⬇️
packages/flutter_tools/lib/src/base/process.dart	`82.94% <0%> (+0.77%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e31d88f...566f1d4. Read the comment docs.

jonahwilliams · 2019-08-20T23:45:19Z

cc @goderbauer @Piinks

jonahwilliams · 2019-08-21T00:14:36Z

packages/flutter/lib/src/foundation/_consolidate_response_web.dart

+///
+/// This implementation is unsupported and throws when invoked.
+Future<Uint8List> consolidateHttpClientResponseBytes(
+  Object response, {


This type can't be HttpClient since that comes from dart:io

Could we make it be a Stream<List<int>>?

Not sure what that would accomplish. You won't get a Stream<List> from an XmlHttpRequest, so if you're calling this method at all you've done something wrong

jonahwilliams · 2019-08-21T00:15:23Z

packages/flutter/lib/src/material/about.dart

  // can provide an explicit applicationName to the widgets defined in this
  // file, instead of relying on the default.
  final Title ancestorTitle = context.ancestorWidgetOfExactType(Title);
-  return ancestorTitle?.title ?? Platform.resolvedExecutable.split(Platform.pathSeparator).last;


This returns /bin/system-something on release mode android

jonahwilliams · 2019-08-21T00:38:37Z

packages/flutter/lib/src/services/text_input.dart

        return true;
      }
-      if (Platform.isIOS) {
+      if (defaultTargetPlatform == TargetPlatform.iOS) {


Looks like we have a lot of tests that rely on this being false for both :(

I think these asserts were essentially never tested. Maybe we should remove them?

The tester would resolve this to the host platform right?

We should fix the test. I believe the asserts work as intended even if they're not correctly tested. And this should be a switch.

this is definitely not default Target Platform fodder. Changing the UI style to Android doesn't magically make it support iOS actions.

dnfield · 2019-08-21T02:24:45Z

examples/flutter_gallery/lib/demo/video_demo.dart


 Future<bool> isIOSSimulator() async {
-  return Platform.isIOS && !(await deviceInfoPlugin.iosInfo).isPhysicalDevice;
+  return defaultTargetPlatform == TargetPlatform.iOS && !(await deviceInfoPlugin.iosInfo).isPhysicalDevice;


If we switch to TargetPlatform, this should be a switch.

default Target Platform is wrong here. It does not reliably say if you're on the iOS simulator: it just says what your preferred UI style is. you can toggle it by pressing "o" at the console in flutter run, for instance.

Ahh yes. But we could still expose something like TargetPlatform currentPlatform somewhere and have that delegate to Platform when io is available

dnfield · 2019-08-21T02:27:43Z

packages/flutter/lib/src/painting/image_provider.dart


  /// The file to decode into an image.
-  final File file;
+  final Object file;


Could we come up with a foundation type for this instead of Object? Maybe for a future PR.

I think anything else (besides dynamic) is actually a breaking change. If the type implements File, then dart:io is still a dependency of the API. If it doesn't, then it breaks callsites.

If we decide to make a breaking change, it would be easier to maintain this API if it took a path String instead.

dnfield · 2019-08-21T02:29:33Z

packages/flutter/lib/src/widgets/focus_manager.dart

    // This only affects the initial value: the ongoing value is updated as soon
    // as any input events are received.
-    _lastInteractionWasTouch ??= Platform.isAndroid || Platform.isIOS || !WidgetsBinding.instance.mouseTracker.mouseIsConnected;
+    _lastInteractionWasTouch ??= defaultTargetPlatform == TargetPlatform.android ||


Hixie · 2019-08-21T02:32:25Z

packages/flutter/lib/src/foundation/consolidate_response.dart

 /// parameter to false.
 Future<Uint8List> consolidateHttpClientResponseBytes(
-  HttpClientResponse response, {
+  Object response, {


I don't think this is ok. we lose type safety too.

Hixie · 2019-08-21T02:33:38Z

packages/flutter/lib/src/material/about.dart

  final Title ancestorTitle = context.ancestorWidgetOfExactType(Title);
-  return ancestorTitle?.title ?? Platform.resolvedExecutable.split(Platform.pathSeparator).last;
+  return ancestorTitle?.title ?? 'Flutter';
 }


Flutter is definitely not correct here.

Updated to use Platform.resolved executable on mobile/desktop and "browser" on the web

Hixie · 2019-08-21T02:40:48Z

packages/flutter/lib/src/widgets/image.dart

  /// If [excludeFromSemantics] is true, then [semanticLabel] will be ignored.
  Image.file(
-    File file, {
+    Object file, {


this seems highly undesireable

Hixie · 2019-08-21T02:42:47Z

packages/flutter/lib/src/services/text_input.dart

        return true;
      }
-      if (Platform.isIOS) {
+      if (defaultTargetPlatform == TargetPlatform.iOS) {


this is definitely not default Target Platform fodder. Changing the UI style to Android doesn't magically make it support iOS actions.

…divergence_on_modules

…illiams/flutter into dart2js_ddc_divergence_on_modules

jonahwilliams · 2019-08-26T22:42:55Z

packages/flutter/lib/src/foundation/platform.dart

+/// The platform that the application is currently running on.
+///
+/// The [currentHostPlatform] getter returns the current platform.
+enum HostPlatform {


I did not use target platform for this since I think I need an enum for each Platform.is*, regardless of whether it is supported.

jonahwilliams · 2019-08-26T22:45:39Z

I've updated this PR with some changes to make the types less bad.

Introduce a "HostPlatform/currentHostPlatform", really just a targetPlatform that can't be overriden. Might be able to use TargetPlatform enum.
Update image.dart to conditional export a child class that has constructors implemented. This requires duplication, only to remove the File type from the API. There is no other way I could think to do this that wasn't breaking or not type safe.
Update consolidateHttpResponse to conditional export and exclude HttpClient from the web api

dnfield · 2019-08-26T23:37:02Z

packages/flutter/lib/src/foundation/_platform_web.dart

+
+/// The dart:html implementation of [platform.currentHostPlatform].
+platform.HostPlatform get currentHostPlatform {
+  return platform.HostPlatform.browser;


Should this resolve to specific browsers? E.g. Safari vs Chrome?

we could return the user agent?

That feels more like a plugin of some sort to me.

HostPlatform.browser feels something more like HostPlatform.mobileApplication instead of HostPlatform.androidor HostPlatform.ios. I'm not really sure what the plans for browser support are, but it seems like it would fit better to have HostPlatform.chrome and HostPlatform.safari here.

There is some slight need for browser detection features, but that is handled entirely in the engine - similarly to how the android/ios shells handle certain platform specific features.

I don't really know that we'd ever need to know safari vs chrome, not to mention with user agent spoofing there are no guarantees that our guess is accurate.

dnfield · 2019-08-26T23:37:38Z

packages/flutter/lib/src/foundation/_exit_web.dart

+///
+/// This is a no-op, as web applications do not have sufficient permissions to
+/// exit their own browser.
+void exit() { }


Should this throw, or at least print an error log?

Not sure, we don't call this today otherwise we would crash on exit.

As a user I might find it confusing if I found this, called it, and had nothing happen. I'd probably go file a bug before reading the documentation.

dnfield · 2019-08-26T23:38:40Z

packages/flutter/lib/src/foundation/platform.dart

+/// Unlike [TargetPlatform], this value is not intended to be overriden
+/// for theming or physics. Instead, it is only for guarding features
+/// which are highly platform specific, such as accessibility adaptions.
+HostPlatform get currentHostPlatform => _platform.currentHostPlatform;


Can we make this a compile time constant so it works with tree shaking?

No, Platform.is* is not a constant so the overall getter can't be const. I'm also pretty sure that redirecting a getter through a const won't tree shake for the case of the browser impl

Maybe for another PR then, but it seems like we should be able to just define these as const .fromEnvironments or something.

dnfield · 2019-08-26T23:51:17Z

packages/flutter/lib/src/services/_network_asset_bundle_web.dart

+    return parser(await loadString(key));
+  }
+
+  // TODO(ianh): Once the underlying network logic learns about caching, we


We don't need this here.

dnfield · 2019-08-26T23:52:34Z

packages/flutter/lib/src/services/_network_asset_bundle_web.dart

+class NetworkAssetBundle extends asset_bundle.AssetBundle implements asset_bundle.NetworkAssetBundle {
+  /// Creates an network asset bundle that resolves asset keys as URLs relative
+  /// to the given base URL.
+  NetworkAssetBundle(this._baseUrl);


Can we throw here for a better stack trace?

Can we give them some suggestion of what they should use instead? Just a NetworkImage?

This seems like it should just be the same as NetworkImage anyway, but it's not clear to me how it's supposed to be different. Either way, I think we should just suggest using NetworkImage which should be portable.

I've updated this to throw. I don't feel great making suggestions here because I don't even know when NetworkAssetBundle would be useful

dnfield · 2019-08-26T23:56:29Z

packages/flutter/lib/src/widgets/_image_web.dart

+    this.filterQuality = FilterQuality.low,
+  }) : loadingBuilder = null,
+       image = null {
+    throw UnsupportedError('Image.file is not supported on the web.');


It would be great if we provided a migration suggestion here, such as Use Image.memory instead.

dnfield · 2019-08-26T23:56:54Z

packages/flutter/lib/src/widgets/focus_manager.dart

  Iterable<FocusNode> get traversalChildren {
    return children.where(
-      (FocusNode node) => !node.skipTraversal && node.canRequestFocus,
+          (FocusNode node) => !node.skipTraversal && node.canRequestFocus,


nit: spacing in this file seems all off. Probably can be reverted?

Hmm.. not sure why these changed. will revert

jonahwilliams · 2019-08-27T20:59:12Z

Updated goldens in flutter_test to conditionally not depend on package:image, which does not support the web

dnfield · 2019-08-27T22:47:40Z

packages/flutter/lib/src/painting/debug.dart

 ///
 /// Used by [debugNetworkImageHttpClientProvider].
-typedef HttpClientProvider = HttpClient Function();
+typedef HttpClientProvider = Object Function();


Since this is for debug, can we just break this and have it return an interface that implements io when it's available?

Updated to export API

dnfield · 2019-08-27T22:49:20Z

packages/flutter/lib/src/widgets/focus_manager.dart

  Iterable<FocusNode> get traversalChildren {
    return children.where(
-      (FocusNode node) => !node.skipTraversal && node.canRequestFocus,
+        (FocusNode node) => !node.skipTraversal && node.canRequestFocus,


This still seems off?

Piinks

Golden changes LGTM. Thanks for doing this. :)
There are some doc changes in the related golden classes that need to be merged.

Piinks · 2019-09-03T16:15:21Z

dev/bots/analyze.dart

+    if (errors.length == 1) {
+      print('${bold}An error was detected when looking at dart:* libraries within the Flutter package:$reset\n');
+    } else {
+      print('${bold}Multiple errors were detected when looking at dart:* librarie within the Flutter package:$reset\n');


Typo nit:

Suggested change

print('${bold}Multiple errors were detected when looking at dart:* librarie within the Flutter package:$reset\n');

print('${bold}Multiple errors were detected when looking at dart:* libraries within the Flutter package:$reset\n');

Piinks · 2019-09-03T16:17:02Z

examples/flutter_gallery/lib/gallery/demos.dart

+  if (dart.library.html) '../demo/video_demo_web.dart';
 import 'icons.dart';

+


Nit: extra line

Piinks · 2019-09-03T16:17:42Z

packages/flutter/lib/src/foundation/_consolidate_response_io.dart

+///
+/// The future returned will forward any error emitted by `response`.
+///
+///


Nit: extra line

Piinks · 2019-09-03T16:29:12Z

packages/flutter/lib/src/services/_network_asset_bundle_io.dart

+
+/// The `dart:io` implementation of [asset_bundle.NetworkAssetBundle].
+class NetworkAssetBundle extends asset_bundle.AssetBundle implements asset_bundle.NetworkAssetBundle {
+  /// Creates an network asset bundle that resolves asset keys as URLs relative


Typo nit:

Suggested change

/// Creates an network asset bundle that resolves asset keys as URLs relative

/// Creates a network asset bundle that resolves asset keys as URLs relative

Piinks · 2019-09-03T16:47:42Z

packages/flutter_test/lib/src/_goldens_io.dart

+
+import 'goldens.dart';
+
+/// The default [GoldenFileComparator] implementation for `flutter test`.


There are doc updates that landed in #39082 for GoldenFileComparator and LocalFileComparator.

Piinks · 2019-09-03T16:49:41Z

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

 const String jsEntrypointSourceMapExtension = '.dart.js.map';
 const String jsEntrypointArchiveExtension = '.dart.js.tar.gz';
 const String digestsEntrypointExtension = '.digests';
+


Nit: extra line

Piinks · 2019-09-03T16:50:07Z

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

        '--define', 'flutter_tools:entrypoint=release=$release',
        '--define', 'flutter_tools:entrypoint=profile=$profile',
        '--define', 'flutter_tools:shell=flutterWebSdk=$flutterWebSdk',
+        '--define', 'flutter_tools:ddc_modules=release=$release'


Nit:

Suggested change

'--define', 'flutter_tools:ddc_modules=release=$release'

'--define', 'flutter_tools:ddc_modules=release=$release',

Piinks · 2019-09-03T16:53:57Z

packages/flutter/lib/src/widgets/focus_manager.dart

  Size get size {
    assert(
-        context != null,
+    context != null,


Nits: It looks like the indentation is off for the changes in this file.

Piinks · 2019-09-03T17:08:15Z

FYI: I realize there are already merge conflicts to resolve here, but I noticed #39589 adds dart:io to packages/flutter/lib/src/widgets/focus_manager.dart 👍

dnfield

LGTM once the nits are fixed

jonahwilliams · 2019-09-09T23:28:58Z

I'm going to break this PR up so it is landable into cleanups of the various separate libraries, starting with flutter_test.

jonahwilliams added 2 commits August 18, 2019 13:52

allow dart2js and ddc to disagree on supported libraries

e7a7660

add editorial

12e36a3

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Aug 18, 2019

googlebot added the cla: yes label Aug 18, 2019

remove dart:io from public API

99d6735

jonahwilliams changed the title ~~Allow dart2js and dartdevk to disagree on what libraries are supported.~~ Remove dart:io/dart:isolate from transitive closure of web applications Aug 20, 2019

Piinks added the framework flutter/packages/flutter repository. See also f: labels. label Aug 21, 2019

fix service extension test

59224b0

jonahwilliams marked this pull request as ready for review August 21, 2019 00:14

jonahwilliams requested a review from goderbauer August 21, 2019 00:14

jonahwilliams commented Aug 21, 2019

View reviewed changes

dnfield reviewed Aug 21, 2019

View reviewed changes

Hixie reviewed Aug 21, 2019

View reviewed changes

jonahwilliams added 5 commits August 20, 2019 22:35

try another approach for image

e30b03b

move platform implementations into foundation

c786cae

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

9982ee7

…divergence_on_modules

Merge branch 'dart2js_ddc_divergence_on_modules' of github.com:jonahw…

7aa85eb

…illiams/flutter into dart2js_ddc_divergence_on_modules

cleanups of exports

46b7b73

jonahwilliams commented Aug 26, 2019

View reviewed changes

jonahwilliams requested review from Hixie and dnfield August 26, 2019 22:43

dnfield reviewed Aug 26, 2019

View reviewed changes

cleanup of flutter test

6b69089

dnfield reviewed Aug 26, 2019

View reviewed changes

Jonah Williams added 2 commits August 26, 2019 17:27

Update _binding_web.dart

e898a38

address some comments

9317da1

jonahwilliams mentioned this pull request Aug 27, 2019

Missing modules exception doesn't list which modules are missing dart-lang/build#2425

Closed

jonahwilliams added 4 commits August 26, 2019 19:53

hide package:image dependency and fix import spelling

76c1c51

re-surface compareLists

c5ebbeb

Update android_device.dart

02e20ab

Update goldens.dart

631fecb

dnfield reviewed Aug 27, 2019

View reviewed changes

jonahwilliams added 2 commits August 28, 2019 10:52

update http client and add test case

ce3db44

dont comment out tests

566f1d4

jonahwilliams mentioned this pull request Sep 3, 2019

Add an option to skip the platform check for specific packages dart-lang/build#2420

Merged

Piinks reviewed Sep 3, 2019

View reviewed changes

dnfield approved these changes Sep 4, 2019

View reviewed changes

jonahwilliams closed this Sep 9, 2019

jonahwilliams mentioned this pull request Sep 10, 2019

Update the supported library set for Flutter for web #39983

Merged

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

	print('${bold}Multiple errors were detected when looking at dart:* librarie within the Flutter package:$reset\n');
	print('${bold}Multiple errors were detected when looking at dart:* libraries within the Flutter package:$reset\n');

		if (dart.library.html) '../demo/video_demo_web.dart';
		import 'icons.dart';

	/// Creates an network asset bundle that resolves asset keys as URLs relative
	/// Creates a network asset bundle that resolves asset keys as URLs relative


		import 'goldens.dart';

		/// The default [GoldenFileComparator] implementation for `flutter test`.

	'--define', 'flutter_tools:ddc_modules=release=$release'
	'--define', 'flutter_tools:ddc_modules=release=$release',

Remove dart:io/dart:isolate from transitive closure of web applications #38773

Remove dart:io/dart:isolate from transitive closure of web applications #38773

Uh oh!

Conversation

jonahwilliams commented Aug 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Background

Overview

Related Issues

Uh oh!

fluttergithubbot commented Aug 18, 2019

Uh oh!

codecov bot commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jonahwilliams commented Aug 20, 2019

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!

jonahwilliams commented Aug 26, 2019

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 Aug 18, 2019 •

edited

Loading

codecov bot commented Aug 19, 2019 •

edited

Loading