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 visionOS support #291

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

harlanhaskins
Copy link

This switches to canImport(UIKit)/canImport(AppKit)/canImport(WatchKit) instead of OS-specific checks in the cases where the only requirement is importing the specific framework and using those symbols.

This also adds an entry into the device type matching for Apple Vision Pro and updates the demo project and tests to also run on visionOS.

💡 Motivation and Context

We're currently unable to use PostHog on visionOS because of compilation issues, and we would like to be able to.

💚 How did you test it?

Added build support to the Demo project for Apple Vision Pro and ran in the simulator. All tests continue to pass. Note: I had to update Nimble to the latest version for the test to be able to build successfully.

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • I updated the docs if needed.
  • No breaking change or entry added to the changelog.

This switches to `canImport(UIKit)`/`canImport(AppKit)`/`canImport(WatchKit)` instead
of OS-specific checks in the cases where the only requirement is importing the specific
framework and using those symbols.

This also adds an entry into the device type matching for Apple Vision Pro.
@marandaneto
Copy link
Member

@harlanhaskins thanks for the PR.

@ioannisj will let you review this.
One thing to pay attention to is when to import ios or uikit, session replay is ios only so we need to be sure we're not leaking any method, view modifier, etc for platforms that have uikit but not session replay yet, otherwise it seems legit.

Copy link
Contributor

@ioannisj ioannisj left a comment

Choose a reason for hiding this comment

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

@harlanhaskins thanks for all the work here!

I’ve left a few comments, mainly because this is currently breaking builds for tvOS/watchOS. UIKit can be imported, but only a subset of it is available and it’s causing issues.

There are also some places where we exposed some view modifiers/utils that are intended for session replay only.

You can run make buildSdk and make buildExamples to test if anything breaks.

Out of curiosity, what build errors are you currently seeing in your visionOS app?

@@ -5,7 +5,7 @@
// Created by Yiannis Josephides on 09/10/2024.
//

#if os(iOS) && canImport(SwiftUI)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's revert this since this is related to session replay/masking which is only available for iOS currently. Since this is a public api, let's expose specifically to iOS

@@ -7,7 +7,7 @@

// Inspired from: https://github.com/siteline/swiftui-introspect

#if os(iOS) && canImport(SwiftUI)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same, let's revert since this is view masking related

@@ -6,7 +6,7 @@
//
// Adapted from: https://github.com/SDWebImage/SDWebImageWebPCoder/blob/master/SDWebImageWebPCoder/Classes/SDImageWebPCoder.m

#if os(iOS)
Copy link
Contributor

Choose a reason for hiding this comment

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

WebP related which is only used by session replay (iOS only)

@@ -5,7 +5,7 @@
// Created by Yiannis Josephides on 11/11/2024.
//

#if os(iOS) || os(tvOS)
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't compile on watchOS since only a subset of UIKit is available.
UIViewController is only available on iOS, iPadOS, Mac Catalyst, tvOS, visionOS

@@ -5,7 +5,7 @@
// Created by Yiannis Josephides on 03/12/2024.
//

#if os(iOS) || os(tvOS)
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to UIApplication+.swift, UIWindow is only available on iOS, iPadOS, Mac Catalyst, tvOS, visionOS

@harlanhaskins
Copy link
Author

Ah, you’re completely right, I did neglect to think of the special position watchOS is in. Honestly I think it’s not worth switching to canImport for “consistency” if we have to break the rule in so many places to explicitly exclude watchOS.

I’ll go through this again and manually add || os(visionOS) where it makes sense instead of blanket using canImport. There are more iOS APIs exposed to visionOS than tvOS so I don’t think it’ll be a find&replace operation.

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.

3 participants