[material_ui] Remove unconditional dart:io import from about.dart - #12774
[material_ui] Remove unconditional dart:io import from about.dart#12774Gyeony95 wants to merge 1 commit into
Conversation
about.dart imported dart:io only to read Platform.resolvedExecutable as the fallback application name. Because about.dart is reachable from the package barrel, pana marks material_ui as not supporting the web and drops the platform:web and is:wasm-ready tags. Move the lookup behind a conditional import so the web build never references dart:io. Fixes flutter/flutter#191887
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
There was a problem hiding this comment.
Code Review
This pull request refactors AboutDialog to remove the unconditional dart:io import, using conditional imports (_about_io.dart and _about_web.dart) to resolve the executable name on web and native platforms. It also adds tests to verify this behavior. The review feedback points out that the test file itself introduces an unconditional dart:io import and accesses Platform.resolvedExecutable, which will cause compilation failures during web testing, and suggests applying conditional imports to the test file as well.
| // found in the LICENSE file. | ||
|
|
||
| import 'dart:async'; | ||
| import 'dart:io' show Platform; |
There was a problem hiding this comment.
Unconditionally importing dart:io in a test file will cause compilation failures when running tests on the web (e.g., via flutter test --platform chrome). Since this PR aims to enable web support, we should avoid unconditional dart:io imports in the test file as well. We can use a conditional import to import the platform-specific executable name helper instead.
| import 'dart:io' show Platform; | |
| import 'package:material_ui/src/_about_io.dart' | |
| if (dart.library.js_interop) 'package:material_ui/src/_about_web.dart' as about; |
There was a problem hiding this comment.
dart:io compiles on the web (it just throws at runtime), so an unconditional import in a test file does not break web test compilation. This file passes with flutter test --platform chrome as-is, and the only test that touches Platform is skipped on the web with skip: kIsWeb. Other tests in this repo (e.g. test/l10n/translations_test.dart) import dart:io the same way. Importing src/_about_io.dart from a test would also couple the test to a private implementation file, so I would prefer to keep the plain import.
| // Regression test for https://github.com/flutter/flutter/issues/191887. | ||
| await tester.pumpWidget(buildWithoutTitle(const AboutDialog())); | ||
|
|
||
| final String expectedName = Platform.resolvedExecutable.split(Platform.pathSeparator).last; |
There was a problem hiding this comment.
Use the conditionally imported about.executableName instead of directly accessing Platform.resolvedExecutable to avoid the compile-time dependency on dart:io on the web.
| final String expectedName = Platform.resolvedExecutable.split(Platform.pathSeparator).last; | |
| final String expectedName = about.executableName; |
There was a problem hiding this comment.
Computing the expected value from Platform.resolvedExecutable directly is intentional: the widget already reads about.executableName, so using the same getter as the expected value would just compare the getter against itself. Deriving it independently keeps the test meaningful. This test is skipped on the web (skip: kIsWeb), so there is no dart:io runtime dependency there.
about.dartimporteddart:iounconditionally, only to readPlatform.resolvedExecutableas the fallback application name when noTitleancestor is found. Becauseabout.dartis reachable from thematerial_uibarrel, pana 0.23.18's static platform analysis marks the whole package as not supporting the web, which drops theplatform:webandis:wasm-readytags on pub.dev. It also means that fallback path throwsUnsupportedErrorwhen it is actually reached on the web.This PR moves the executable-name lookup behind a conditional import (
_about_io.dart/_about_web.dart), following the same patternpackage:flutter's foundation library uses for_platform_io.dart/_platform_web.dart. On the web the fallback name is the empty string, matching the existing empty-string default for the application version. The[Platform.resolvedExecutable]doc references are kept resolvable with a@docImport.Verified locally with pana 0.23.x on this branch:
Supports 6 of 6 possible platforms, and theplatform:web/is:wasm-readytags are back.Fixes flutter/flutter#191887
Pre-Review Checklist
[shared_preferences]///).Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2