twitter / ios-twitter-image-pipeline

Twitter Image Pipeline is a robust and performant image loading and caching framework for iOS clients
Apache License 2.0
1.85k stars 109 forks source link

Add Subspecs for Extended code (WebP and MP4 Codecs) #56

Closed liamnichols closed 4 years ago

liamnichols commented 4 years ago

Hello again @NSProgrammer πŸ‘‹ This PR is a followup to #53 that I hope addresses some initial concerns πŸ™

Now that I'm back to migrating our project over to TwitterImagePipeline i'm back to the drawing board trying to work out how best to integrate TIP along with the WebP codec that you already provide and test in this repo.

Like WEBP_README.md rightly explains, compiling/integrating libwebp into a project isn't as simple as we'd like and I've not been able to come up with an approach to integrate the TIPXWebPCodec into our project in a way that I'm happy with.

When managing the TwitterImagePipeline dependency via something like Git submodules then I understand how including the WebP codec might not be such a big problem however when using CocoaPods to manage the main dependency then configuring the WebP.framework and copying source files over manually seems fragile and a lot of work that can be avoided.

After looking back at your original feedback and the configuration of the project, I realised that there might have been a slight misunderstanding how subspecs work and also I discovered a way to avoid external dependencies that this change demonstrates.

Firstly...

It is important that the WebP extended code be committed to the repo but also unnecessary for using TIP, only Apple provided decoders/encoders will be automatically supported. This will avoid any binary bloating for consumers that do not need codecs that are not included by Apple.

I agree with the statement above but when using CocoaPods, the entire git repository will be cloned regardless to if source files are included in the Default subspec's source_files or not. Prior to this change, you don't include the ./Extended directory in the Podspec so the code will never be compiled and won't bloat the resulting project.

In this change, I add additional subspecs but I don't include them in the default_subspec configuration. This means that out the box, users who declare the TwitterImagePipeline pod will by default only use the Default subspec.

So in a pod file configuration:

pod 'TwitterImagePipeline'
# is the same as 
pod 'TwitterImagePipeline', :subspecs => ['Default']

I've added extra Subspecs in this PR that aren't' included as the default, so by default they are ignored (thus avoiding any binary bloating) however they can be opted into using configurations such as the following:

pod 'TwitterImagePipeline', :subspecs => ['WebPCodec/Default']
pod 'TwitterImagePipeline', :subspecs => ['WebPCodec/Default', 'MP4Codec']
pod 'TwitterImagePipeline', :subspecs => ['WebPCodec/Animated']
pod 'TwitterImagePipeline', :subspecs => ['MP4Codec']

Only when one of the configurations like the above are used would the extended code be included in the binary output and this would only ever be the code that the consumer intentionally wanted.

If it helps, you can see the output Xcode Project that CocoaPods produces for somebody opting into WebPCodec/Animated and MP4Codec subspecs below:

Screenshot 2020-08-20 at 09 48 34

We are committed to having Twitter Image Pipeline be a zero dependency project

This is a great thing to be doing, and in my original proposal I suggested using the libwebp pod however now I've realised that wasn't necessary..

In this proposed solution, I use vendored_frameworks to reference the precompiled WebP and WebPDemux frameworks instead. This is possible since the Podspec currently is only configured for iOS and from what I understand CocoaPods doesn't actually support Catalyst (CocoaPods/CocoaPods#8877) so for the time being this solution works fine.

If you do also want to support Mac via CocoaPods then it isn't impossible either.. Since you already include the source in the repo, we'd just need to add another subspec with a configuration similar to this: https://github.com/SDWebImage/libwebp-Xcode/blob/master/libwebp.podspec and use that instead (although in the long run, I think we'll want to push to work on an .xcframework version of libwebp (I think)).


Anyway, please take a look at this change and the above and let me know what you think.. Since it's zero dependencies and requires no code changes I hope it resolves some of your prior concerns but let me know if there is more πŸ™

I was also thinking of adding some info the readme but seeing as there is already no mention of CocoaPods I didn't know if it was worth it or not.. Let me know if you do want me to document anything though.

Lastly, here is the pod spec lint output (all is good):

Output ``` -> TwitterImagePipeline (2.24.0) - NOTE | [TwitterImagePipeline/Default, TwitterImagePipeline/WebPCodec, TwitterImagePipeline/WebPCodec/Default, and more...] xcodebuild: note: Using new build system - NOTE | [TwitterImagePipeline/Default, TwitterImagePipeline/WebPCodec, TwitterImagePipeline/WebPCodec/Default, and more...] xcodebuild: note: Building targets in parallel - NOTE | [TwitterImagePipeline/Default, TwitterImagePipeline/WebPCodec, TwitterImagePipeline/WebPCodec/Default, and more...] xcodebuild: note: Planning build - NOTE | [TwitterImagePipeline/Default, TwitterImagePipeline/WebPCodec, TwitterImagePipeline/WebPCodec/Default, and more...] xcodebuild: note: Constructing build description - NOTE | [iOS] xcodebuild: note: Build description constructed in 0.209491149s - NOTE | [iOS] xcodebuild: note: Using build description 'ad760b0efabd4862f1636d529a9228aa' - NOTE | [TwitterImagePipeline/Default, TwitterImagePipeline/WebPCodec, TwitterImagePipeline/WebPCodec/Default, and more...] xcodebuild: note: Using eager compilation - NOTE | [TwitterImagePipeline/Default, TwitterImagePipeline/WebPCodec, TwitterImagePipeline/WebPCodec/Default, and more...] xcodebuild: warning: Skipping code signing because the target does not have an Info.plist file and one is not being generated automatically. (in target 'App' from project 'App') - NOTE | [iOS] [TwitterImagePipeline/Default] xcodebuild: note: Build description constructed in 0.179841603s - NOTE | [iOS] [TwitterImagePipeline/Default] xcodebuild: note: Using build description 'd884f8e2d5f66a1cf8dd2b6065860e9d' - NOTE | [iOS] [TwitterImagePipeline/WebPCodec] xcodebuild: note: Build description constructed in 0.177936516s - NOTE | [iOS] [TwitterImagePipeline/WebPCodec] xcodebuild: note: Using build description '9d59a6ab73ce4416f340a9a979c3aa4c' - NOTE | [iOS] [TwitterImagePipeline/WebPCodec/Default] xcodebuild: note: Build description constructed in 0.177591027s - NOTE | [iOS] [TwitterImagePipeline/WebPCodec/Default] xcodebuild: note: Using build description '046535f7d04f66a39129b59a714a5d4d' - NOTE | [iOS] [TwitterImagePipeline/WebPCodec/Animated] xcodebuild: note: Build description constructed in 0.176152623s - NOTE | [iOS] [TwitterImagePipeline/WebPCodec/Animated] xcodebuild: note: Using build description '161247a73b43f2db73d7b5a7fc0b87f8' - NOTE | [iOS] [TwitterImagePipeline/MP4Codec] xcodebuild: note: Build description constructed in 0.179207421s - NOTE | [iOS] [TwitterImagePipeline/MP4Codec] xcodebuild: note: Using build description '3fb25302f4156ee441f260649ea287af' Analyzed 1 podspec. TwitterImagePipeline.podspec passed validation. ```
NSProgrammer commented 4 years ago

This is great! I am going to merge this as-is, however I think it would be helpful to add a COCOAPODS.md doc that goes over how to use Twitter Image Pipeline with CocoaPods.

liamnichols commented 4 years ago

Amazing thanks! I’ll try to draft up a document for CocoaPods usage at some point tomorrow πŸš€