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

Add static platform check props #37870

Closed
wants to merge 1 commit into from

Conversation

ryanlntn
Copy link
Contributor

Summary:

I've noticed that a lot of RN projects have something along the lines of the following in them:

export const isAndroid = Platform.OS === 'android'
export const isIOS = Platform.OS === 'ios'

This seems common enough that I think it makes sense to just export these from RN itself. Platform already has isPad and isTV. This would add another way to express platform checks with that API.

Changelog:

[GENERAL] [ADDED] - Adds isAndroid, isIOS, isMacOS, isNative, isWeb, and isWindows static properties to Platform

Test Plan:

I just patched these into a demo app. Here's a screenshot:

Screenshot 2023-06-13 at 7 39 06 PM

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Infinite Red Partner: Infinite Red Partner labels Jun 14, 2023
@motiz88 motiz88 self-requested a review June 14, 2023 01:54
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,755,105 -147
android hermes armeabi-v7a 8,067,667 -159
android hermes x86 9,247,692 -161
android hermes x86_64 9,096,829 -156
android jsc arm64-v8a 9,316,217 -243
android jsc armeabi-v7a 8,506,127 -231
android jsc x86 9,379,701 -246
android jsc x86_64 9,632,951 -238

Base commit: c870a52
Branch: main

Comment on lines +23 to +28
isAndroid: true,
isIOS: false,
isMacOS: false,
isNative: true,
isWeb: false,
isWindows: false,
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be awesome!
But, this is kind of "redundant" since:

  1. Android and iOS will use and read from their respective files.
  2. If the platform is Android this would implicitly mean it's not iOS, etc., and vice-versa?
  3. Why would an Android app would need a check for MacOS, native, web, and Win similarly for iOS app?
  4. If we want to support (check) all these platforms then we should create another generic file (interface with "all props optional") that will define/expose them.

WDYT?
P.S.: I may be unaware of the full context here :)

Copy link
Contributor Author

@ryanlntn ryanlntn Jun 14, 2023

Choose a reason for hiding this comment

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

It is certainly redundant. I've just noticed that many RN projects have the same const isAndroid = Platform.OS === 'android' declared and thought there may as well be a utility for devs to express one off platform checks this way offered directly by RN.

@Pranav-yadav Pranav-yadav added the Needs: Docs Website PR To encourage the authors to create a website PR for documentation. label Jun 14, 2023
Copy link
Contributor

@NickGerleman NickGerleman left a comment

Choose a reason for hiding this comment

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

The current design here is that OS is a string constant, defined per platform. This allows platforms to work out-of-tree, public or private, by defining their own constant. So, the mechanism you use to check for iOS/Android is the same whether you are developing on a phone, on desktop, or a PlayStation. This is something we lose with this change.

It also means we need to update this centralized list, and if a platform loses support, we need to either break API, or keep around a dead interface.

Some of that has leaked into the TypeScript typings in this repo that used to be on DefinitelyTyped, but out-of-tree platforms like React Native Windows have more recently gained general ability to re-export their own types, and tell tsc to resolve types for a specific platform during typechecking.

@motiz88 pointed out offline that Metro has Babel transforms which use Platform.OS conditions in the AST to strip code for platforms not being bundled for. So switching to these helpers would defeat the dead code elimination.

@ryanlntn
Copy link
Contributor Author

@NickGerleman Good to know. What do you think about something like Platform.is('ios') that just wraps the Platform.OS condition? That could use PlatformOSType for now and would still read similar. Not sure if Babel transforms would still be a problem with that.

@motiz88
Copy link
Contributor

motiz88 commented Jun 14, 2023

@ryanlntn

What do you think about something like Platform.is('ios') that just wraps the Platform.OS condition?

My main question here is "why?" What does this achieve from a user's perspective that Platform.OS === 'ios' doesn't?

I don't mean to sound harsh. It's just that, considering the extra effort required to optimise and then teach this proposed new API, I'm afraid such an API wouldn't be all that valuable. (If anything we should be looking at ways to deprecate and then remove the existing Platform.isPad and Platform.isTV.)

I think we'd certainly be open to a new API if one is warranted, but we should probably discuss the motivation and design in an RFC before going any further with an implementation.

@ryanlntn
Copy link
Contributor Author

@motiz88

My main question here is "why?" What does this achieve from a user's perspective that Platform.OS === 'ios' doesn't?

In terms of functionality it doesn't accomplish anything new. And yet as stated above almost every RN project I've seen has something along the lines of const isAndroid = Platform.OS === 'android' in it. So there's something about expressing one off platform checks that way that appeals to people.

I don't mean to sound harsh. It's just that, considering the extra effort required to optimise and then teach this proposed new API, I'm afraid such an API wouldn't be all that valuable. (If anything we should be looking at ways to deprecate and then remove the existing Platform.isPad and Platform.isTV.)

I'm curious about these optimizations. I was unaware that there was a babel transform stripping dead code based on Platform.OS conditions. Would people assigning const isAndroid = Platform.OS === 'android' and checking against isAndroid break that or would it still work in that case? Is this documented somewhere?

I think we'd certainly be open to a new API if one is warranted, but we should probably discuss the motivation and design in an RFC before going any further with an implementation.

Fair enough.

@jamonholmgren
Copy link
Collaborator

jamonholmgren commented Jun 14, 2023

Can confirm, having isAndroid etc is definitely common among our clients over the past 7+ years of doing RN consulting.

@ryanlntn
Copy link
Contributor Author

Opened an RFC.

@NickGerleman
Copy link
Contributor

I'm curious about these optimizations. I was unaware that there was a babel transform stripping dead code based on Platform.OS conditions. Would people assigning const isAndroid = Platform.OS === 'android' and checking against isAndroid break that or would it still work in that case? Is this documented somewhere?

I'm not sure this is documented, but here is a link to where Platform.OS is replaced with the platform being bundled for. https://github.com/facebook/metro/blob/c6a94bc170cf95a6bb21b5638929ec3311a9a5b7/packages/metro-transform-plugins/src/inline-plugin.js#L152

So, a condition might be transformed to a constant comparison like if ('android' === 'ios') { which is is able to be optimized away. I think the same would apply to the above example, but I am less familiar with the other transforms and optimizations to know if it has the same overall effect if used in a condition (Moti may know more). You can always inspect the bundled JS to know for sure what was emitted.

I tend to agree though, that adding Platform.is('android') doesn't seem to enable more than the existing Platform.OS === 'android', beyond saving a couple of keystrokes.

Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Dec 12, 2023
Copy link

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Needs: Docs Website PR To encourage the authors to create a website PR for documentation. p: Infinite Red Partner: Infinite Red Partner Stale There has been a lack of activity on this issue and it may be closed soon.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants