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.

Sign up for GitHub

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

Jump to bottom

Making AnyItemModel implement ErasedContentProviding. #156

Merged
merged 4 commits into from
Nov 21, 2023
Merged

Making AnyItemModel implement ErasedContentProviding. #156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
be18827
Making AnyItemModel implement ErasedContentProviding.
Oct 31, 2023
c68476d
CHANGELOG.md update.
Oct 31, 2023
5bb8217
Merge branch 'master' of github.com:airbnb/epoxy-ios into rafael_AnyI…
Oct 31, 2023
13add00
Fixing lint error.
Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
## [Unreleased](https://github.com/airbnb/epoxy-ios/compare/0.10.0...HEAD)

### Changed
- ...
- `AnyItemModel` now implements the `ErasedContentProviding` protocol.
- Updated `ErasedContentProviding` protocol to use its type name instead of `Self` in the keys of its `EpoxyModelProperty` properties `contentProperty` and `isContentEqualProperty `.

### Fixed
- Fixed an issue causing incorrect view callbacks and a corresponding assertion when a view
Expand Down
4 changes: 4 additions & 0 deletions Sources/EpoxyCollectionView/Models/ItemModel/AnyItemModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


extension AnyItemModel: ErasedContentProviding { }

// MARK: WillDisplayProviding

extension AnyItemModel: WillDisplayProviding { }
Expand Down
10 changes: 5 additions & 5 deletions Sources/EpoxyCore/Model/Providers/ErasedContentProviding.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}

// MARK: - EpoxyModeled
Expand All @@ -32,18 +32,18 @@ extension EpoxyModeled where Self: 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.
public var isErasedContentEqual: ((Self) -> Bool)? {
public var isErasedContentEqual: ((ErasedContentProviding) -> Bool)? {
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.

}

private var isContentEqualProperty: EpoxyModelProperty<((Self) -> Bool)?> {
.init(keyPath: \Self.isErasedContentEqual, defaultValue: nil, updateStrategy: .replace)
private var isContentEqualProperty: EpoxyModelProperty<((ErasedContentProviding) -> Bool)?> {
.init(keyPath: \ErasedContentProviding.isErasedContentEqual, defaultValue: nil, updateStrategy: .replace)
}
}
Loading