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

fix(ios, deviceType): support iOS-compiled app running on macOS #1181

Merged

Conversation

user-none
Copy link
Contributor

Description

Expansion of PR #1137. That PR only checked if the app is an iOS app built for macOS not if it is an iOS app running on macOS. See https://developer.apple.com/documentation/xcode/creating_a_mac_version_of_your_ipad_app for details about how Catalyst is for creating a macOS native type package from an iOS app. #1137 uses a compile time define which is only present when creating a macOS version.

This adds a runtime check for running unmodified iOS apps on macOS, https://developer.apple.com/documentation/apple-silicon/running-your-ios-apps-on-macos . This allows proper detection of the underlying device type the iOS app is running on.

Compatibility

OS Implemented
iOS
Android
Windows

Checklist

  • [ X] I have tested this on a device/simulator for each compatible OS

mikehardy
mikehardy previously approved these changes Mar 17, 2021
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Clever! This looks great to me. Out of curiosity - since you're in the area, do you see anything easy to do for #1178 ? Perhaps just an @available check ? I think the same issue could affect your change, but I'm not sure how concerned to be - I don't know the relative adoption of various catalyst versions

@mikehardy
Copy link
Collaborator

@user-none - just pending a question about being even more compatible than existing code already is, nothing wrong with the code as is I like this solution

@user-none
Copy link
Contributor Author

@mikehardy #1178 is easy enough to fix. Give me a bit and I'll update this PR with the code for it.

@mikehardy
Copy link
Collaborator

Champion! Seriously, thank you

@user-none
Copy link
Contributor Author

I found a bug in my code while looking at the other ticket. I've pushed up a fix but I'm still working on the other one.

@user-none
Copy link
Contributor Author

The best I can come up with is:

#if __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000
    case UIUserInterfaceIdiomMac: return DeviceTypeDesktop;
#endif

This should work because Catalyst tracks iOS versions. The header for enum UIUserInterfaceIdiom is an iOS header and specifically sets UIUserInterfaceIdiomMac as available in iOS 14. I said should work because I can't test it. The current version of Xcode ships with Catalyst 14 and when I create a new project it doesn't allow targeting for Catalyst 13.

I can commit that if you don't want, or you can make 14 a min dependency going forward.

@mikehardy
Copy link
Collaborator

I'm inclined to just say Catalyst 14 is a minimum version, Xcode 12 is already a minimum version and I am fine in general with the statement "You must run current stable Xcode to develop things for Apple products", then enforce minimums based on that.

At the end of the day if it's a really serious issue, that's why patch-package exists, and why we all use open source. You can just fix it, and it's not even that hard.

Thanks for looking into this I do appreciate it

So then I think (please confirm) there is actually nothing else actionable here, and I should close the other issue re v13 catalyst while merging and releasing this. I think that should match your thinking?

@user-none
Copy link
Contributor Author

At the end of the day if it's a really serious issue ...

The code to work around it is in this conversations so if it needs to be dealt with, we know what to do.

So then I think (please confirm) there is actually nothing else actionable here, and I should close the other issue re v13 catalyst while merging and releasing this. I think that should match your thinking?

Nothing else actionable. It's ready for merging. Closing out #1178 is appropriate since the 14 is the minimum required catalyst version.

@mikehardy mikehardy changed the title run time check for iOS app running on macOS. fix(ios, deviceType): support iOS-compiled app running on macOS Mar 17, 2021
@mikehardy mikehardy merged commit 3ff676c into react-native-device-info:master Mar 17, 2021
@mikehardy
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants