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

Alamofire: Upgrade or remove? #19043

Closed
mokagio opened this issue Jul 14, 2022 · 4 comments
Closed

Alamofire: Upgrade or remove? #19043

mokagio opened this issue Jul 14, 2022 · 4 comments
Labels
Tooling Build, Release, and Validation Tools [Type] Core

Comments

@mokagio
Copy link
Contributor

mokagio commented Jul 14, 2022

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 mokagio added Tooling Build, Release, and Validation Tools [Type] Core labels Jul 14, 2022
@mokagio
Copy link
Contributor Author

mokagio commented Jul 14, 2022

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:

WordPress-iOS/Podfile.lock

Lines 440 to 443 in 23ae9e7

- RNFastImage (8.5.11):
- React-Core
- SDWebImage (~> 5.11.1)
- SDWebImageWebPCoder (~> 0.8.4)

...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
Copy link
Contributor Author

mokagio commented Jul 14, 2022

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

See wordpress-mobile/gutenberg-mobile#5025

@derekblank
Copy link
Contributor

derekblank commented Jul 14, 2022

Good catch! Yep, FastImage is coming as part of WordPress/gutenberg#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
Copy link
Contributor Author

mokagio commented Nov 13, 2024

@crazytonyli addressed this through the upgrade path in #15244

@mokagio mokagio closed this as completed Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tooling Build, Release, and Validation Tools [Type] Core
Projects
None yet
Development

No branches or pull requests

2 participants