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

Making AnyItemModel implement ErasedContentProviding. #156

Conversation

rafael-assis
Copy link
Contributor

@rafael-assis rafael-assis commented Oct 31, 2023

Change summary

This PR changes AnyItemModel to implement the ErasedContentProviding protocol.

It also changes the ErasedcontentProviding protocol to use its type name instead of Self in the keys of its EpoxyModelProperty properties contentProperty and isContentEqualProperty .

Context

We found this gap through a test failure that wasn't able to find a SwiftUI View used to build items of a SectionModel.
The reason for that is that these SwiftUI Views were placed in the SectionModel as AnyItemModel instances created from the call to eraseToAnyItemModel() method in the ItemModeling protocol:

extension AnyItemModel: ItemModeling {
  public func eraseToAnyItemModel() -> AnyItemModel { self }
}

Our test framework has an implementation similar to the snippet below to find SwiftUI Views.
The commented (labeled "TODO: Support AnyItemModel") code below is what would need to be added to support these type erased item models that contain SwiftUI Views.

  /// - Note: The underlying model must conform to `EpoxyCore.ErasedContentProviding` for a SwiftUI `View` to
  /// be found.
  public func swiftUIView<T: View>(of _: T.Type) -> T? {
    let erasedContentProviding = {

//      TODO: Support AnyItemModel
//      if let anyItemModel = model as? AnyItemModel {
//        return anyItemModel.model as? (any ErasedContentProviding)
//      }

      return model as? (any ErasedContentProviding)
    }()

    let content = erasedContentProviding?.erasedContent as? EpoxySwiftUIHostingView<T>.Content
    return content?.rootView
  }
}

That additional commented code would be needed since AnyItemModel did not implement ErasedContentProviding but as a type eraser, wrapped a model that did.

We then concluded that all we would need is to make AnyItemModel implement ErasedContentProviding and the issue would be resolved. But the SwiftUI View still wasn't found in the test as the contentProperty was always nil.

So, we asked ourselves:
But doesn't AnyItemModel provide a passthrough storage property to its backing model property when called directly? (snippet below)

public struct AnyItemModel: EpoxyModeled {
  ... 
  public var model: InternalItemModeling

  /// Implemented as a passthrough to the backing model's storage to allow custom model properties
  /// to be accessed and modified through this type eraser.
  public var storage: EpoxyModelStorage {
    get { model.storage }
    set { model.storage = newValue }
  }
  ...
}

That's where changing the EpoxyModelProperty keys from Self to ErasedContentProviding comes in:

Storing the keys with Self would always resolve to the type name that implements ErasedContentProviding.
That means when the backing model storage was created, it would have the key path as something like KeyPath<EpoxyCollectionView.ItemModel<SwiftUIViewName>>.

When the test tried to retrieve that same property from the storage of type EpoxyModelStorage using its subscript getter, the key passed would be KeyPath<EpoxyCollectionView.AnyItemModel>.

As the keys mismatched, the return was always the default value (nil in this case) and the SwiftUI View was never found from the test query.

Using ErasedContentProviding instead of Self ensures that key path will be always consistent for the property on both the creation and retrieval times. They key will be something like KeyPath<EpoxyCore.ErasedContentProviding>.

How was it tested?

How did you verify that this change accomplished what you expected? Add more detail as needed.

  • Wrote automated tests
  • Built and ran on the iOS simulator
  • Built and ran on a device
  • Ran test module of our private code that finds the SwiftUI View in an Epoxy Section Model.

Pull request checklist

All items in this checklist must be completed before a pull request will be reviewed.

  • Risky changes have been put behind a feature flag, e.g. CollectionViewConfiguration
  • Added a CHANGELOG.md entry in the "Unreleased" section for any library changes

Copy link

@bachand bachand left a comment

Choose a reason for hiding this comment

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

This change feels correct to me given this comment on AnyItemModel. Thanks @rafael-assis !

/// Implemented as a passthrough to the backing model's storage to allow custom model properties
/// to be accessed and modified through this type eraser.
public var storage: EpoxyModelStorage {
get { model.storage }
set { model.storage = newValue }
}

@@ -33,6 +33,10 @@ public struct AnyItemModel: EpoxyModeled {

}

// MARK: ErasedContentProviding

extension AnyItemModel: ErasedContentProviding {}
Copy link

Choose a reason for hiding this comment

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

Mimics this conformance:

// MARK: ErasedContentProviding
extension ItemModel: ErasedContentProviding { }

get { self[isContentEqualProperty] }
set { self[isContentEqualProperty] = newValue }
}

// MARK: Private

private var contentProperty: EpoxyModelProperty<Any?> {
.init(keyPath: \Self.erasedContent, defaultValue: nil, updateStrategy: .replace)
.init(keyPath: \ErasedContentProviding.erasedContent, defaultValue: nil, updateStrategy: .replace)
Copy link

Choose a reason for hiding this comment

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

I wondered if changing the keys could break existing code. I see that EpoxyModelStorage does not provide a way to enumerate keys, which increases my confidence that this change is safe.

@@ -14,7 +14,7 @@ public protocol ErasedContentProviding {
/// A closure that can be called to determine whether the given `model`'s `erasedContent` is equal
/// to this model's `erasedContent`, else `nil` if there is no content or the content is always
/// equal.
var isErasedContentEqual: ((Self) -> Bool)? { get }
var isErasedContentEqual: ((ErasedContentProviding) -> Bool)? { get }
Copy link

Choose a reason for hiding this comment

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

This is a very subtle change that could affect implementations, since now we don't guarantee at compile-time that the types we are comparing match. It would be nice if we did a search for how we are using this property internally to validate that we think this change is safe.

@@ -33,6 +33,10 @@ public struct AnyItemModel: EpoxyModeled {

}

// MARK: ErasedContentProviding
Copy link

Choose a reason for hiding this comment

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

@thedrick I see that you added this file. Do you have any thoughts on this change?

Copy link
Contributor

Choose a reason for hiding this comment

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

@bachand no comments, it's entirely possible that this file used to be something totally different and the header was never updated. Eric is the one who wrote all of this code

Copy link

Choose a reason for hiding this comment

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

Thanks Tyler.

@brynbodayle brynbodayle merged commit ecee1ac into airbnb:master Nov 21, 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.

4 participants