-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[Help wanted]SDWebImage 6.0 Proposal: Rewriten Swift API with the overlay framework instead of Objective-C exported one #2980
Comments
This is the draft proposal. I need some SDWebImage's Swift user about the design and idea for this. You can leave comment here about this. The SDWebImage 6.0 development is this 2020 year's goal. |
We can learn more from Facebook iOS SDK what they do: https://github.com/facebook/facebook-ios-sdk It create a separate Swift Module(target) for Swift user, new naming and Swift Helper code. I think, SDWebImage can be as complicated as Facebook SDK if it grows up. Re-write entirely using Swift does not help for anything (either on performance, or safety), because both of them already solved by using the lowever C API and thread safe lock (Which Swift can not magically solve). The main pros is that Swift become more attractive for beginner iOS developer. However, for decoding && transformer && code which need the deep domain knowledge, is not easy for beginner iOS developer to write. Re-write them using Swift is as ugly as Objective-C 😅 So, provide a Swift friendly API is much more meaningful than totally rewriten by copying line to line in Swift. |
@dreampiggy Facebook used to have a separate Swift wrapper around their Objc-C SDK and they have deprecated this Swift wrapper: https://github.com/facebookarchive/facebook-swift-sdk/blob/master/Announcement.md My 2 cents:
|
Yes. I think it does not help so much if we rewrite SD with Swift. None of the things changed in performance if you're a SDWebImage users. Only change the way of the SDWebImage developer and future maintainer. Framework user and framework author are different. My proposal is simple for user:
Which means, from 6.0, we have 3 type of users:
|
If someone agree that it's hard to maintain such 3 types of combination is a good way out. We can simply to remove the support for type 2 user. All SwiftUI have to use new API instead. This is what Facebook SDK team they choose to do. |
Overlay framework naming, i thought is better. |
@dreampiggy I think it can become a bit difficult to maintain 2 sets of Swift APIs. I would like to hear some more opinions here @SDWebImage/collaborators |
Not a extra git repo. One git repo, but have to be 2 Xcode target. This is because of suck of SwiftPM. |
For CocoaPods, we can choose to use Subspec, or create one new Podspec for this. The difference is how user import. # SDWebImage.podspec
s.subspec `Core` do |ss|
ss.sources_files = 'SDWebImage/Classes/**/*'
end
s.subspec `Swift` do |ss|
ss.sources_files = 'SDWebImage/Swift/**/*'
end or # SwiftWebImage.podpsec
s.sources_files = 'SDWebImage/Swift/**/*' You can check Facebook SDK again, see what they do: https://github.com/facebook/facebook-ios-sdk/blob/master/FacebookSDK.podspec They choose the seond one. ( My idea: If we use subspec (the first approach), user from SwiftPM can use import SDWebImage // They can import, because SwiftPM have two targets
import SwiftWebImage However, user from CocoaPods have to use import SDWebImage
// import SwiftWebImage // They can't, beucase there are no SwiftWebImage module. The `SDWebImage/Swift` subspec module name is still `SDWebImage` This is because CocoaPods does not allow subspec to override the Which may cause confusing, because now they are different thing. Swift standard library use the same name to acutaly overlay the original one (Their |
Seems Facebook team assume : Their SwiftPM only export the things This is not what I want and disagree with. Swift Package Manager does allows distribution of Objective-C/C/C++ code, it's not any reason to disable Objective-C user from using it. |
They mention this for Objective-C and Swift user, to use the different import: |
Like I said, Facebook deprecated their separate Swift framework, as it creates extra confusion (and other reasons). Also, I think adding the Swift API in the form of a separate framework that requires an extra import is just overhead, especially for people that use CocoaPods or Carthage with default settings (which mean building frameworks as dynamic). Most of the users fall into those categories and an extra framework means a slower startup time. So my suggestion is:
|
SDWebImage and SwiftWebImage are not separate framework. They are same one. Swift user's our framework module name is
Maybe we can't, after I re-think the case again. The SwiftPM will become the main iOS package manager in the next few years, all of CocoaPods && Carthage will disappear in history. We have to support it. SwiftPM don't use subspec, and I don't think CocoaPods subspec is correct. Subspec is the tool about combination related components, but not about components which have strict dependency. The So, we should place For both CocoaPods or SwiftPM user, they should use the same import syntax, just
The If Swift user really do this It just like Swift user import |
All this complicated issue is because of this limit:
See: https://developer.apple.com/documentation/xcode/creating_a_standalone_swift_package_with_xcode This current limit makes it's not possible to add some polished API (need pure Swift code, but not only some SwiftPM source code about this: https://github.com/apple/swift-package-manager/blob/master/Sources/PackageLoading/TargetSourcesBuilder.swift#L138-L141 |
Let's assume if we follows your suggestion to use CocoaPods's subspec:
CocoaPods user have to use SDWebImage new API using: import SDWebImage
imageView.sd.setImage(with: buffer, options: [.priority(.high)]) { result
switch(result) {
//...
}
}
SwiftPM user have to use SDWebImage new API using: import SwiftWebImage
imageView.sd.setImage(with: buffer, options: [.priority(.high)]) { result
switch(result) {
//...
}
} See ? The problem is : For end user, now I have to write different code to import the same framework, just because of the Package Manager I choose.
Things also applied for our downstream dependency, like SDWebImageSwiftUI. Because SDWebImageSwiftUI supports both CocoaPods and SwiftPM as well. Now I have to write like this: #if canImport(SDWebImage)
import SDWebImage
typealias SDModuleName = SDWebImage // Have to do so
#endif
#if canImport(SwiftWebImage)
import SwiftWebImage
typealias SDModuleName = SwiftWebImage // Have to do so
#endif
public struct AnimatedImage: UIVIewRepresentable {
typealias UIViewType = SDModuleName.AnimatedImageView // Really tricky
} As the author and maintainer of SDWebImageSwiftUI, I don't think it's a good solution, which make things more complicated for maintain. Each Package Manager should use the Same Code to import and use for end-user. |
Another framework to reference about Swift API transition: PSPDFKit They used to have true overlay framework, but now merged into it's own mainstream codebase. Note: They support Carthage/CocoaPods, but not SwiftPM. They put the mixed Objecitive-C and Swift code into a single framework target. This, of cource, will force the Objecitive-C only client have to ship with Swift Runtime binary 7MB size, which it's not good. I don't think pure Objecitve-C user must make their ipa size become bigger because someone supports better Swift API, they don't gain what they paid. |
Maybe, we can just mixed coding with Objc and Swift into a single "Target". No actual "overlay". Put "Overlay" on logic, but not actual a target. The Swift and Objc source code is mixed into compile unit and produce single Mach-O Library. Which means, the Package.swift may looks like: .library(name: "_SDWebImageCore", dependencies: [], path: "SDWebImage", sources: ["Core", "Private"]) // implements details, Objc code only
.library(name: "SDWebImage", dependencies: ["_SDWebImageCore"], path: "Swift") And, for CocoaPods, there are no subspec. Just put all source code of Objc/Swift into I'll have a try tomorrow. |
This all sounds great. I'm a swift user. Once there is a swifty API I won't have any interest in the clang imported Obj-C -> swift API. I don't see any benefit to maintaining that if that causes any extra development time or test time. I currently use Cocoapods and would like to move to using SPM but haven't yet. The SPM team does read and respond to forum posts at forums.swift.org so one can post feature requests there and pain points. |
Great @phoney The thing is SPM does gain traction and more and more users will be migrating to it as a default dependency management system.
Given all those reasons and more, I think CocoaPods will be here for a few more years at least, so our goal as open source contributors is to support SPM and CocoaPods in equal measures, one can't take precedence over the other. Not to mention Carthage is also a compatibility I would aim to keep. They just give people more options. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, please make sure it is up to date and if so, add a comment that this is still an issue to keep it open. Thank you for your contributions. |
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, please make sure it is up to date and if so, add a comment that this is still an issue to keep it open. Thank you for your contributions. |
Assuming this improved Swift API is still on the table, I wanted to request structured-concurrency equivalents to asynchronous methods in SDWebImage. For each current method that takes a completion handler closure, we would love to have an For example, this method call:
would have an
For the time being, Swift developers can use the new
Let me know if you would prefer I make a new issue for this specific request. https://docs.swift.org/swift-book/LanguageGuide/Concurrency.html |
Seems too long after the first proposal... My personal job and open-source work may related to some other compiler-based stack (not iOS UI level) 😓 But it's still really welcomed to achieve this for any interested community contributors and users 👋 For example, if you think this is useable, you can drive some sample about the new API looks like, and some implementation can be easily achive via overlay framework tech I talked above. Or, we can make it two-way to achieve:
|
Recall with the roadmap to SDWebImage 6.0 See draft implementation in https://github.com/dreampiggy/SDWebImage/tree/swift Which means: SDWebImage 6.0 will become You can not use SDWebImage without linking the Or, maybe Xcode 15's require after WWDC 23 |
@dreampiggy I can work on this. I think the best way to achieve this is to start from scratch by bringing pieces of code to the Swift version, like Apple is doing with Foundation. On a second guess, we can work on this as a fork, like you've already is doing 😅. |
To keep everybody updated, I'm working on it SDWebImage rewritten in Swift. Currently is 8.3%! I think the best way is to deliver the code in Swift format first, run the tests and after start making improvements. I've done this in other private projects and it's very overwhelming adding new features while refactoring the code. |
Hi. Thanks for your support !!! Is that rewritten in Swift means, the pure Swift implementation ? Or just the API refinement ? Because at the original design or RFC, I want to support both Swift / Objc users of SDWebImage. So at that https://github.com/dreampiggy/SDWebImage/tree/swift, I only rewrite the |
Which means, we'd better, for example: For this objc UIImageView.sd_setImageWithURL call `__sd_setImageWithURL` internal method swift UIImageView.sd.setImage(with:) call `__sd_setImageWithURL` internal method __sd_setImageWithURL is a global objc(or even C) method, which need to extract the Swift associated enum object from Swift, and convert into the internal form object, then dispatch to next job. So, as you can see, both Swift/Objc users can still use their API (but which is designed differently for each languages and use the best features their languages support). Just like Apple's Foundation, there are still Objc/Swift two APIs, but the Swift API looks Swifty on Swift site (we does not use the Objc generated Interface, but the interface written by our hand) For class, actually we can hide the For that stupid |
Hmm, I understand now the proposal. I didn’t read the details and assumed that this required the code being rewritten in Swift. I think the idea of making methods exclusive for Swift just to make the API not objc style can be an unnecessary work. As you explain very well, this changes will deliver a new package that calls the objc code, but using Swift on the surface. However, I'll search for more details to keep contributing with the original proposal. Thanks for clarifying @dreampiggy! 🫶🏻 |
Background
The current SDWebImage 5.0 Swift API is exported from Objective-C interface by clang importor. Which looks OK, but not really convenient compared with other pure Swift API. For example:
This looks not so Swifty in Swift world. A better design should looks like this, by using the powerful of Swift Language syntax, including:
Note: The
options
arg is a enum with associated object, which combine theSDWebImageOptions
andSDWebImageContext
together, don't need to separate. Objective-C API has to use two type because of the pool C enum limitation to bind to Int value.Solution
To achieve this goal of polished Swift API, we have some choices.
Modify the Objective-C source code interface with
NS_SWIFT_NAME
This can solve the cases like renaming, for example:
SDImageCache
can renamed intoSDWebImage.ImageCache
, which drop the prefix ofSD
However, this can not solve the issue like
SDWebImageOptions
+SDWebImageContext
into the new APISDWebImage.ImageOptions
enum cases.Create another framework (overlay) and rewrite API
See: https://pspdfkit.com/blog/2018/first-class-swift-api-for-objective-c-frameworks/
See: https://forums.swift.org/t/how-apple-sdk-overlays-work/11317
See: https://github.com/apple/swift/blob/06685bcb516942d6b4ae2cb0c6be5ce324029898/stdlib/public/Darwin/Network/Network.swift
This can be done like this: Create a new framework called
SwiftWebImage
(naming can be discuessed), which have a source files namedSDWebImage.swift
and have the following contents:@_exported import SDWebImage
Then, maked all the public API in SDWebImage as
@unavailable
, like this:Finally, implements the overlay logic by calling the original SDWebImage API, like this (the
sd
wrapper is from Kingfisher's design, actually the same) :Done. When you import the
SwiftWebImage
framework, your original SDWebImage old Swift API will be marked as unavailable, so you can enjoy the new API.import SwiftWebIamge import SDWebImage // This will be overlayed and not visible, actuallly you don't need to import this
Overlay framework naming
Question: For Apple Standard Framework like
Network.framework
, the Swift Runtime provide a overlay framework which module name is the same asNetwork
. So if you write :You actually don't import the
Network.framework
, but import thelibSwiftNetwork.dylib
and its module.import SwiftNetwork // Actually what you do
We want to adopt this design as well, so that you don't need to replace the
import SDWebImage
intoimport SwiftWebImage
. However, due to the reality that we support 3 different package manager:prepare_script
script phase to copy the module nameAnd, for exist SDWebImage 5.0 user, if he/she really don't want to update to the new Swifty API, they can still use
import SDWebImage
to use the old Objective-C exported API. Naming the overlay framework different fromSDWebImage
can allows for this choice.Which means: there are two types of Swift API, depends on how you import:
Rewrite SDWebImage totally with Swift and drop Objective-C user
As my personal option, this is not always a good idea. There are already some awesome Swift-only Image Loading framerwork for iOS community:
We have some common design and solution for the same thing. Rewrite entirely need some more timeing and unit testing to ensure function.
And, SDWebImage 5.0 current is still popular in many Objective-C project and users, expecially in China. Nearly 80% users from Objective-C use it in the company's App (not personal App). And most of them have a codebase which mixed the two language. Drop these Objective-C user need to be carefuly considerated.
And I think: Currently, Objective-C and Swift is a implementation details, the benefit from Swift written in implementation may including:
So, my personal idea for SDWebImage 6.0.0, it's that we only rewrite the API level for Swift, still using Objecitive-C for internel implementation. Both Swift and Objective-C shall share the same function and optimization from version upgrade.
The text was updated successfully, but these errors were encountered: