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 macOS and tvOS support to EpoxyCore #143

Merged
merged 16 commits into from
Jul 5, 2023
Merged

Conversation

calda
Copy link
Member

@calda calda commented Jun 28, 2023

Opening this PR as a discussion starter. Consider this a proof-of-concept.

Starting in airbnb/lottie-ios#2092 we're using EpoxyCore's SwiftUI UIViewConfiguringSwiftUIView to provide a SwiftUI wrapper for Lottie's LottieAnimationView UIView.

EpoxyCore currently only officially supports iOS, but Lottie also supports tvOS and macOS.

  • tvOS uses UIKit, so EpoxyCore already supports tvOS with basically no code changes.
  • macOS uses AppKit, so there are a decent number of differences

If EpoxyCore supported macOS, we could provide Lottie SwiftUI support on all Apple platforms. This lets you, for example, create a single SwiftUI app which uses Lottie and can be ran on all Apple platforms.

There are two types of changes that have to be made to support macOS / AppKit:

  1. Replace references of UIView with NSView (etc). This PR implements that with a typealias UIViewOrNSView which resolves to either UIView or NSView depending on the platform. Most of EpoxyCore compiles successfully with just these changes.
  2. Add #if os(macOS) as necessary where UIView and NSView have different APIs or behaviors. The most important example of this is in SwiftUIMeasurementContainer, where the behavior has to be slightly different on macOS.

To me these changes feel relatively noninvasive. It's especially reassuring that there are only a few places where we have to implement different runtime behavior depending on the platform.

What do folks think?

If we're ok with this direction I'll work on further productionizing these changes, e.g. by adding more code comments and by adding CI jobs that test the tvOS and macOS support.


Example screenshots from a multiplatform SwiftUI app using Lottie, running on macOS and tvOS with a single codebase using the changes in this PR:

macOS / AppKit

Screenshot 2023-06-28 at 11 42 34 AM

tvOS / UIKit

Screenshot 2023-06-28 at 11 43 05 AM

Source code of example:

struct ContentView: View {
  var body: some View {
    HStack {
      VStack {
        LottieView(animation: LottieAnimation.named("Samples/LottieLogo1")!)
          .looping()
          .frame(maxWidth: 100)

        Text("maxWidth: 100")
      }

      VStack {
        LottieView(animation: LottieAnimation.named("Samples/LottieLogo1")!)
          .looping()
          .frame(maxHeight: 100)

        Text("maxHeight: 100")
      }

      VStack {
        LottieView(animation: LottieAnimation.named("Samples/LottieLogo1")!)
          .looping()
          .resizable()

        Text("resizable")
      }

      VStack {
        LottieView(animation: LottieAnimation.named("Samples/LottieLogo1")!)
          .looping()

        Text("intrinsic content size")
      }
    }
    .ignoresSafeArea()
    .frame(maxWidth: .infinity, maxHeight: .infinity)
  }
}

Copy link
Contributor

@thedrick thedrick left a comment

Choose a reason for hiding this comment

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

This is really awesome @calda I made some comments and am curious what your thoughts are. We will want want this to be a major version bump if we do end up making breaking naming changes

@brynbodayle
Copy link
Contributor

Overall approach LGTM

@calda calda marked this pull request as ready for review June 29, 2023 19:51
@calda
Copy link
Member Author

calda commented Jun 29, 2023

This PR is ready for review now @airbnb/epoxy-ios-maintainers

### Fixed
- ...

## [0.10.0](https://github.com/airbnb/epoxy-ios/compare/0.9.0...0.10.0) - 2023-06-29
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we release these changes as 0.10.0 after merging this PR or should I leave it under "Unreleased"? (Somebody else would have to cut the release since I don't have write access)

Copy link
Contributor

Choose a reason for hiding this comment

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

@calda I'm wondering if we should cut a release before these changes, and then do a new release after these changes have been merged so we can include the other non-breaking changes currently unreleased?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, that makes sense. We could release 0.9.1 and then 0.10.0?

Copy link
Member Author

@calda calda Jun 29, 2023

Choose a reason for hiding this comment

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

Well, looking at the CHANGELOG we've only every cut releases like 0.6.0, 0.7.0, 0.8.0, 0.9.0. So I guess this should be 0.10.0 without the breaking changes (not 0.9.1) and then 0.11.0 with the breaking changes?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here you go: #144

@brynbodayle brynbodayle merged commit fbcae5c into airbnb:master Jul 5, 2023
9 checks passed
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.

None yet

3 participants