Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: improve platform check by using constants and defaultTargetPlatform #2188

Merged
merged 3 commits into from
Sep 7, 2024

Conversation

EchoEllet
Copy link
Collaborator

@EchoEllet EchoEllet commented Sep 5, 2024

Description

Improve the platform check by using kIsWeb and defaultTargetPlatform to allow it to be tested and tree shaken.

  • kIsWeb will always be true if kIsWasm is in case of Flutter/WASM
  • The main reason for using defaultTargetPlatform over Platform.operatingSystem from `dart: is that it can be tested and overridden, and supports the web in case we want to check if the platform regardless if it's on a web browser.
  • Recently (about 7 months ago), defaultTargetPlatform has been updated to use vm:platform-const-if pragma introduced in dart-lang/sdk@57a1168875 in Flutter #141105 which allow it to be computed as if it was a constant field in non-debug builds which means platform-specific code executed conditionally based on defaultTargetPlatform can be tree-shaken in release builds
  • Added properties such as isIos which will check if the operating system is iOS which will be true even on the web, isIosApp will require the platform to be an iOS app and non-web to be true:
bool get isIos => defaultTargetPlatform == TargetPlatform.iOS;
bool get isIosApp => !kIsWeb && isIos;

Also added @pragma('vm:platform-const-if', !kDebugMode) on each property, kIsWeb is a constant field, defaultTargetPlatform is constant only on non-debug builds, I'm uncertain if we need the pragma annotation for each field (e.g. isAndroidApp or isMobileApp) to be computed as constants and tree shaken same way if we use defaultTargetPlatform directly.

If we take a look at the fileplatform.dart from dart:io, all fields that are using operatingSystem:

  @pragma("vm:platform-const")
  static final operatingSystem = _Platform.operatingSystem;

are also using @pragma("vm:platform-const"):

  @pragma("vm:platform-const")
  static final bool isLinux = (operatingSystem == "linux");

  @pragma("vm:platform-const")
  static final bool isIOS = (operatingSystem == "ios");

  // Etc...

Related Issues

I expect some regressions from this PR, as such it's not ready for merge yet.

Also see:

Type of Change

  • New feature: Adds new functionality without breaking existing features.
  • 🛠️ Bug fix: Resolves an issue without altering current behavior.
  • 🧹 Code refactor: Code restructuring that does not affect behavior.
  • Breaking change: Alters existing functionality and requires updates.
  • 🧪 Tests: Adds new tests or modifies existing tests.
  • 📝 Documentation: Updates or additions to documentation.
  • 🗑️ Chore: Routine tasks, or maintenance.
  • Build configuration change: Changes to build or deploy processes.

Comment on lines -1194 to +1190
if (isKeyboardOS(supportWeb: true)) {
if (isKeyboardOS) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't understood what this check mean by keyboardOS, I assume it's an operating system with a built-in physical keyboard. It checks if it's desktop or fuchsia OS and fuchsia OS doesn't come with a built-in keyboard AFAIK.

Comment on lines -622 to +610
final platform = Theme.of(_state.context).platform;
if (isAppleOS(
platform: platform,
supportWeb: true,
)) {
if (Theme.of(_state.context).isCupertino) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Many of the checks here depend on ThemeData from MaterialApp. Usually, it should be for styling purposes so I created an extension called isCupertino to be a bit clear.

We need to double-check if this platform-specific logic shouldn't be used when overriding the platform with an Apple operating system on Android or other platforms.

@EchoEllet EchoEllet marked this pull request as ready for review September 6, 2024 13:22
@AtlasAutocode
Copy link
Collaborator

This PR clears up a lot of the confusion on implementing platform specific options. I have never understood supportWeb: and anything that removes the confusion is good.

Comment:
One point that may lead to confusion is the close similarity of isIOS and isIos, for example. When you use intellisense to auto enter code, it might be random which gets picked. Also, as I get involved in other programming tasks, I tend to forget the distinction and why I should pick one over the other.

The impact on tree-shaking is something I have not considered but will be very important when there is a platform-specific option that could increase program size by multi-megabytes.

I note that text_selection.dart and delegate.dart are already using defaultTargetPlatform.

@EchoEllet
Copy link
Collaborator Author

This PR clears up a lot of the confusion on implementing platform specific options. I have never understood supportWeb: and anything that removes the confusion is good.

If supportWeb is true and we're on iOS and check if we're on iOS, then it will return true on the web, otherwise will only return true on standlone iOS app and not on web browser, useful when you want to use platform specific features that can't be on other platforms. The name is a bit confusing.

One point that may lead to confusion is the close similarity of isIOS and isIos, for example. When you use intellisense to auto enter code, it might be random which gets picked. Also, as I get involved in other programming tasks, I tend to forget the distinction and why I should pick one over the other.

The choice is a bit random, which leads to inconsistency since we will choose something different each time. I decided to stick to iOS since it's more common.

The impact on tree-shaking is something I have not considered but will be very important when there is a platform-specific option that could increase program size by multi-megabytes.

Usually, this will be useful to help improve the performance a bit where using platform check extensively, for example, when building platform adaptive application with a lot of check-in build method.

I note that text_selection.dart and delegate.dart are already using defaultTargetPlatform.

The platform.dart was already using defaultTargetPlatform, Previously, I didn't know there was a way to override the platform for tests. Tree shaking for this property wasn't a feature.

Copy link
Collaborator

@AtlasAutocode AtlasAutocode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your comments. Looks good.

@singerdmx singerdmx merged commit 151cba5 into master Sep 7, 2024
2 checks passed
@EchoEllet EchoEllet deleted the chore/platform-check branch September 14, 2024 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants