AlamofireImageExtended Types #394
Conversation
| guard | ||
| let strongSelf = self, | ||
| strongSelf.isURLRequestURLEqualToActiveRequestURL(response.request) && | ||
| strongSelf.af_activeRequestReceipt?.receiptID == downloadID |
There was a problem hiding this comment.
Had to remove the weakify/strongify logic here since the container is a struct. Not entirely sure how that ends up working now since I capture self. Thoughts on this @jshier?
There was a problem hiding this comment.
I think it should be okay, but you should see if it can be tested. Perhaps through a local weak reference, or a subclass?
There was a problem hiding this comment.
Wrote some tests and verified we are now capturing it. Found a pretty slick solution in 6335e8d by weaking the image view itself, then capturing imageView?.af as strongSelf in the closure.
There was a problem hiding this comment.
👍 This looks great, and I think it's mergeable as is, but I had a few issues for consideration:
- It would be nice to use the Alamofire extension types, especially since we're still using the
afnamespace here. It would require some properties made public but reduces duplication. If you don't think it's worth that, that's fine though, I don't think we'll collide between the libraries. - Some sort of the test for the previously weak capturing closures might be a good idea to ensure we don't produce any new reference cycles. I don't think it's an issue, but if the tests are easy enough it would be worthwhile.
- I'm not 100% sure, but it might be worthwhile to explore
@dynamicMemberLookupfor the extension type, as I think it would help get rid of sometypeproperty usage.
Otherwise, this is great, and the deprecations should be very helpful.
Source/AlamofireImageExtended.swift
Outdated
| // | ||
|
|
||
| /// Type that acts as a generic extension point for all `AlamofireImageExtended` types. | ||
| public struct AlamofireImageExtension<ExtendedType> { |
There was a problem hiding this comment.
Aside from needing to make type and init public, we could use AlamofireExtended instead here, and I'd hoped to when adding it to Alamofire. Since we're reusing the af extension point anyway, not duplicating the extension methodology seems like a good idea.
| guard | ||
| let strongSelf = self, | ||
| strongSelf.isURLRequestURLEqualToActiveRequestURL(response.request) && | ||
| strongSelf.af_activeRequestReceipt?.receiptID == downloadID |
There was a problem hiding this comment.
I think it should be okay, but you should see if it can be tested. Perhaps through a local weak reference, or a subclass?
| @@ -400,12 +394,14 @@ extension UIImageView { | |||
| /// | |||
| /// - parameter imageTransition: The image transition to ran on the image view. | |||
| /// - parameter image: The image to use for the image transition. | |||
| public func run(_ imageTransition: ImageTransition, with image: Image) { | |||
| public func run(_ imageTransition: UIImageView.ImageTransition, with image: Image) { | |||
| let imageView = type | |||
There was a problem hiding this comment.
Why the local reference here?
There was a problem hiding this comment.
Eliminated the need to capture self in the closure and seemed to read better.
|
|
||
| // Generate a unique download id to check whether the active request has changed while downloading | ||
| let downloadID = UUID().uuidString | ||
|
|
||
| // Weakify the image view to allow it to go out-of-memory while download is running if deallocated | ||
| weak var imageView = self.type |
* Updated af_ to use af. syntax with AlamofireImageExtended types * Updated image view and button extensions to weakify during image download * Updated Alamofire dependency to 5.0.1 and removed AlamofireImageExtended type
Issue Link 🔗
Issue #197
Goals ⚽
To convert all
af_APIs over toaf.syntax using anAlamofireImageExtendedtype.Implementation Details 🚧
There are a couple things worth calling out in this PR that are different than what we had to do in Alamofire itself.
nonmutatingkeyword for the instance setter. Then you have to use the class type itself for the class getters/setters for associated properties.af_APIs rather than remove them. We should probably keep them around and remove them in AFI 5.0. Thoughts?Testing Details 🔍
Test suite is passing with all except the two blur tests that are still failing.