-
Notifications
You must be signed in to change notification settings - Fork 1k
chore: improve platform check by using constants and defaultTargetPlatform #2188
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
Conversation
| if (isKeyboardOS(supportWeb: true)) { | ||
| if (isKeyboardOS) { |
There was a problem hiding this comment.
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.
| final platform = Theme.of(_state.context).platform; | ||
| if (isAppleOS( | ||
| platform: platform, | ||
| supportWeb: true, | ||
| )) { | ||
| if (Theme.of(_state.context).isCupertino) { |
There was a problem hiding this comment.
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.
|
This PR clears up a lot of the confusion on implementing platform specific options. I have never understood Comment: 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. |
If
The choice is a bit random, which leads to inconsistency since we will choose something different each time. I decided to stick to
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
The |
AtlasAutocode
left a comment
There was a problem hiding this 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.
Description
Improve the platform check by using
kIsWebanddefaultTargetPlatformto allow it to be tested and tree shaken.kIsWebwill always betrueifkIsWasmis in case of Flutter/WASMdefaultTargetPlatformoverPlatform.operatingSystemfrom `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.defaultTargetPlatformhas been updated to usevm:platform-const-ifpragma 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 ondefaultTargetPlatformcan be tree-shaken in release buildsisIoswhich will check if the operating system is iOS which will betrueeven on the web,isIosAppwill require the platform to be an iOS app and non-web to betrue:Also added
@pragma('vm:platform-const-if', !kDebugMode)on each property,kIsWebis a constant field,defaultTargetPlatformis constant only on non-debug builds, I'm uncertain if we need the pragma annotation for each field (e.g.isAndroidApporisMobileApp) to be computed as constants and tree shaken same way if we usedefaultTargetPlatformdirectly.If we take a look at the file
platform.dartfromdart:io, all fields that are usingoperatingSystem:are also using
@pragma("vm:platform-const"):Related Issues
I expect some regressions from this PR, as such it's not ready for merge yet.
Also see:
defaultTargetPlatformdocsType of Change