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

[expo-av][iOS] Fix compilation on tvOS #24864

Merged
merged 3 commits into from
Oct 17, 2023

Conversation

douglowder
Copy link
Contributor

Why

Allow the expo-av module to compile on Apple TV.

How

To safely do this without heavy modification of EXAV.m, the podspec is modified so that tvOS uses a separate module implementation EXAVTV.m, with the audio recording code removed and stub module methods for those features.

Test Plan

  • Tested on a test app with Expo 50 alpha packages and the TV plugin
  • CI should continue to pass

Checklist

@linear
Copy link

linear bot commented Oct 13, 2023

ENG-10042 Add Apple TV support to Expo SDK

Now that React Native TV repo is at 0.72.4, even with core, we can start adding partial support for Apple TV.

Included:

  • Support for the modules used in all apps created with create-expo-app
  • Support for expo-updates
  • Support for expo-av (minus recording features)
  • Support for expo-apple-authentication
  • Custom template for TV (maybe expo-template-tv)

Possibly included in future:

  • expo-router
  • expo-dev-client and friends

Excluded:

  • Modules specifically for phone features, or web features (no web on Apple TV)
    • expo-camera
    • expo-barcode-scanner
    • expo-battery
    • expo-cellular
    • expo-contacts
    • expo-face-detector
    • expo-haptics
    • expo-local-authentication
    • expo-location
    • expo-screen-capture
    • expo-screen-orientation
    • expo-sensors
    • expo-speech
    • expo-standard-web-crypto
    • expo-status-bar
    • expo-web-browser

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Oct 13, 2023
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Oct 13, 2023
Copy link
Contributor

@Kudo Kudo left a comment

Choose a reason for hiding this comment

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

looks good. didn't check the code in EXAVTV.m though. it would be even better if we in the future could split shared code into base class like EXAVBase.m.

@tsapeta
Copy link
Member

tsapeta commented Oct 14, 2023

Since we mostly just need to remove methods related to audio, I'm more in favor of not supporting tvOS in expo-av and do this in our new experimental expo-video instead. That should be much easier and I believe we don't really want to maintain tvOS support in both, especially that the API of the latter package will be much different (and already in Swift).

@douglowder
Copy link
Contributor Author

@tsapeta since expo-video work is going to take some time, I would propose this:

  • In this PR, add expo-av to the existing CI that checks compilation on tvOS
  • Go ahead and merge this PR
  • Once expo-video is ready, either modify expo-av to mark it deprecated on tvOS, or simply revert this PR.

What do you think?

@douglowder douglowder force-pushed the doug/eng-10042-add-apple-tv-support-to-expo-sdk branch from 5dc5f0c to 2e235b7 Compare October 16, 2023 17:40
@expo-bot expo-bot added bot: suggestions ExpoBot has some suggestions and removed bot: passed checks ExpoBot has nothing to complain about labels Oct 16, 2023
@wschurman wschurman removed their request for review October 16, 2023 22:21
@douglowder douglowder force-pushed the doug/eng-10042-add-apple-tv-support-to-expo-sdk branch from 2e235b7 to 6bec08c Compare October 17, 2023 15:42
@expo-bot
Copy link
Collaborator

Hi there! 👋 I'm a bot whose goal is to ensure your contributions meet our guidelines.

I've found some issues in your pull request that should be addressed (click on them for more details) 👇

⚠️ Suggestion: Missing changelog entries


Your changes should be noted in the changelog. Read Updating Changelogs guide and consider adding an appropriate entry to the following changelogs:


Generated by ExpoBot 🤖 against 6bec08c

@douglowder douglowder merged commit 17161dc into main Oct 17, 2023
10 of 11 checks passed
@douglowder douglowder deleted the doug/eng-10042-add-apple-tv-support-to-expo-sdk branch October 17, 2023 16:09
marklawlor pushed a commit that referenced this pull request Oct 30, 2023
# Why

Allow the `expo-av` module to compile on Apple TV.

# How

To safely do this without heavy modification of `EXAV.m`, the podspec is
modified so that tvOS uses a separate module implementation `EXAVTV.m`,
with the audio recording code removed and stub module methods for those
features.

# Test Plan

- Tested on a test app with Expo 50 alpha packages and the TV plugin
- CI should continue to pass

# Checklist

- [x] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [x] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [x] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants