Use Gravatar.ImageCaching protocol instead of defining our own #146
Conversation
| public protocol ImageCaching: GravatarImageCaching { | ||
| func setImage(_ image: UIImage, forKey key: String) | ||
| func getImage(forKey key: String) -> UIImage? | ||
| } | ||
|
|
There was a problem hiding this comment.
Is there a reason we were creating our own protocol in WPUI that conforms to the Gravatar protocol? After the renaming, this got tricky. And it seemed to make sense to drop this in favor of using the Gravatar.ImageCaching protocol.
There was a problem hiding this comment.
This probably existed before Gravatar SDK, and now we have the same name for the same definition of protocol.
I agree, we can drop it unless it's used somewhere else as argument independently from Gravatar, which doesn't seem to be the case 🤔
There was a problem hiding this comment.
Is there a reason we were creating our own protocol in WPUI that conforms to the Gravatar protocol?
We didn't. This was here already.
Unfortunately we can't just drop a public type from an open source library. We can't easily know where it's been used. We should always deprecate first even if we want to remove it in the future.
There was a problem hiding this comment.
Unfortunately we can't just drop a public type from an open source library
Oh that's right!
One alternative, if we don't want to re-define the procotol, we can do this:
@_exported import protocol Gravatar.ImageCaching
With @_exported, WPUI will re-export ImageCaching as public, and the API won't change.
Making the changes on WPiOS not needed.
There was a problem hiding this comment.
Oh nice! Didn't know about this. Perfection. I'll push this. Thanks!
| public protocol ImageCaching: GravatarImageCaching { | ||
| func setImage(_ image: UIImage, forKey key: String) | ||
| func getImage(forKey key: String) -> UIImage? | ||
| } | ||
|
|
There was a problem hiding this comment.
This probably existed before Gravatar SDK, and now we have the same name for the same definition of protocol.
I agree, we can drop it unless it's used somewhere else as argument independently from Gravatar, which doesn't seem to be the case 🤔
There was a problem hiding this comment.
Check out https://github.com/wordpress-mobile/WordPressUI-iOS/pull/146/files#r1500335800
We will need one more change 🙏
andrewdmontgomery
merged commit 3de7421
into
task/gravatar-integration
This updates the module so that it implements (and re-exports) the
Gravatar.ImageCachingprotocol instead of a local protocol that conforms to it.Note that since this preserves the
WordPressUI-iOSpublic interfaces, no changes toWordPress-iOSshould be necessary.This should be tested via the WordPress-iOS PR that requires it:
wordpress-mobile/WordPress-iOS#22663
Checkout this branch of
WordPressUI-iOSCheckout Gravatar branch: Automattic/Gravatar-SDK-iOS#50
Checkout WordPress-iOS task/gravatar-integration branch: https://github.com/wordpress-mobile/WordPress-iOS/tree/task/gravatar-integration
Follow the steps mentioned here: wordpress-mobile/WordPress-iOS#22543 (comment)
CHANGELOG.mdif necessary.