wordpress-mobile / WordPress-iOS

WordPress for iOS - Official repository
http://ios.wordpress.org/
GNU General Public License v2.0
3.69k stars 1.12k forks source link

Alamofire: Upgrade or remove? #19043

Closed mokagio closed 4 hours ago

mokagio commented 2 years ago

Alamofire, AlamofireImage, and AlamofireNetworkActivityIndicator are one major version behind. pod outdated returns:

- Alamofire 4.8.0 -> 4.8.0 (latest version 5.6.1)
- AlamofireImage 3.5.2 -> 3.5.2 (latest version 4.2.0)
- AlamofireNetworkActivityIndicator 2.4.0 -> 2.4.0 (latest version 3.1.0)

Before considering whether we should upgrade them, I'd like to know if we actually need them. git grep Alamo ':!Podfile*' returns:

WordPress/Classes/Extensions/UIImageView+SiteIcon.swift:import AlamofireImage
WordPress/Classes/Extensions/UIImageView+SiteIcon.swift:import Alamofire
WordPress/Classes/Services/MediaCoordinator.swift:import enum Alamofire.AFError
WordPress/Classes/Services/MediaCoordinator.swift:        // I don't want to hand-encode the Alamofire.AFError domain and/or code — they're both subject to change
WordPress/Classes/System/WordPressAppDelegate.swift:import AlamofireNetworkActivityIndicator
WordPress/Classes/Utility/Media/ImageLoader.swift:import AlamofireImage
WordPress/Classes/Utility/Universal Links/Routes+Mbar.swift:import Alamofire
WordPress/Classes/Utility/Universal Links/Routes+Mbar.swift:        Alamofire.request(url)
WordPress/Classes/ViewRelated/Gutenberg/GutenbergNetworking.swift:import Alamofire
WordPress/Classes/ViewRelated/Gutenberg/GutenbergNetworking.swift:/// This is needed since AlamoFire will return code: 7 for any error.
WordPress/Classes/ViewRelated/Media/Tenor/TenorAPI/TenorClient.swift:import Alamofire
WordPress/Classes/ViewRelated/Media/Tenor/TenorAPI/TenorClient.swift:        Alamofire.request(url, method: .get, parameters: params).responseData { response in
WordPress/Classes/ViewRelated/Reader/ReaderCrossPostCell.swift:import AlamofireImage
WordPress/Classes/ViewRelated/Site Creation/Site Segments/SiteSegmentsCell.swift:import Alamofire

From that, it seems like the surface area is limited. Having said that, it might also be too much work to remove the dependency. Plus, it works, and it's good to offload tasks like efficiently downloading images to a 3rd party... I don't have a particular desire to keep or remove it, but I'd love for us to be intentional about the choice we take.

mokagio commented 2 years ago

Note that the pod outdated also signals outdated versions of SDWebImage and SDWebImageWebPCoder.

Given SDWebImage and AlamofireImage solve the same problem, it would be nice to keep only one of them.

Unfortunately, SDWebImage comes as a RNFastimage dependencies, so we can't get rid of them: https://github.com/wordpress-mobile/WordPress-iOS/blob/23ae9e7bfe2d92ecd861a1995434190090609f69/Podfile.lock#L440-L443

...Unless we get rid of RNFastImage as well? git grep doesn't show any usage of that framework and neither does find Pods -type f -print0 | xargs -0 grep -li RNFastImage

$ find Pods -type f -print0 | xargs -0 grep -li RNFastImage

Pods/Pods.xcodeproj/project.pbxproj
Pods/Pods.xcodeproj/xcuserdata/gio.xcuserdatad/xcschemes/RNFastImage.xcscheme
Pods/Pods.xcodeproj/xcuserdata/gio.xcuserdatad/xcschemes/xcschememanagement.plist
Pods/Target Support Files/Pods-WordPressTest/Pods-WordPressTest.release-alpha.xcconfig
Pods/Target Support Files/Pods-WordPressTest/Pods-WordPressTest.release.xcconfig
Pods/Target Support Files/Pods-WordPressTest/Pods-WordPressTest.debug.xcconfig
Pods/Target Support Files/Pods-WordPressTest/Pods-WordPressTest.release-internal.xcconfig
Pods/Target Support Files/Pods-Apps-Jetpack/Pods-Apps-Jetpack-acknowledgements.markdown
Pods/Target Support Files/Pods-Apps-Jetpack/Pods-Apps-Jetpack.debug.xcconfig
Pods/Target Support Files/Pods-Apps-Jetpack/Pods-Apps-Jetpack.release.xcconfig
Pods/Target Support Files/Pods-Apps-Jetpack/Pods-Apps-Jetpack.release-internal.xcconfig
Pods/Target Support Files/Pods-Apps-Jetpack/Pods-Apps-Jetpack-acknowledgements.plist
Pods/Target Support Files/Pods-Apps-Jetpack/Pods-Apps-Jetpack.release-alpha.xcconfig
Pods/Target Support Files/Pods-Apps-WordPress/Pods-Apps-WordPress.release.xcconfig
Pods/Target Support Files/Pods-Apps-WordPress/Pods-Apps-WordPress-acknowledgements.markdown
Pods/Target Support Files/Pods-Apps-WordPress/Pods-Apps-WordPress.debug.xcconfig
Pods/Target Support Files/Pods-Apps-WordPress/Pods-Apps-WordPress.release-alpha.xcconfig
Pods/Target Support Files/Pods-Apps-WordPress/Pods-Apps-WordPress-acknowledgements.plist
Pods/Target Support Files/Pods-Apps-WordPress/acknowledgements.html
Pods/Target Support Files/Pods-Apps-WordPress/Pods-Apps-WordPress.release-internal.xcconfig
Pods/Target Support Files/RNFastImage/RNFastImage.modulemap
Pods/Target Support Files/RNFastImage/RNFastImage.debug.xcconfig
Pods/Target Support Files/RNFastImage/RNFastImage-umbrella.h
Pods/Target Support Files/RNFastImage/RNFastImage.release.xcconfig
Pods/Target Support Files/RNFastImage/RNFastImage-dummy.m
Pods/Manifest.lock
Pods/Local Podspecs/RNFastImage.podspec.json

This is something we'll need to verify with the Gutenberg mobile folks.

mokagio commented 2 years ago

This is something we'll need to verify with the Gutenberg mobile folks.

See https://github.com/wordpress-mobile/gutenberg-mobile/issues/5025

derekblank commented 2 years ago

Good catch! Yep, FastImage is coming as part of https://github.com/WordPress/gutenberg/pull/42009.

I'm not too familiar with Alamofire or AlamofireImage, but for some extra context on the RNFastImage dependencies, FastImage is indeed a wrapper around SDWebImage (and the Android equivalent library, Glide). It allows both native libraries to be used in an easy cross-platform context within Gutenberg/Gutenberg Mobile. If you have concerns about overlap of the SDWebImage and AlamofireImage dependencies within WordPress-iOS, however, definitely let's discuss further. 👍

mokagio commented 4 hours ago

@crazytonyli addressed this through the upgrade path in #15244