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

Feature KF builder #1546

Merged
merged 8 commits into from
Oct 4, 2020
Merged

Feature KF builder #1546

merged 8 commits into from
Oct 4, 2020

Conversation

onevcat
Copy link
Owner

@onevcat onevcat commented Oct 4, 2020

This implements #1518

@onevcat onevcat merged commit 023be4f into master Oct 4, 2020
@onevcat onevcat deleted the feature/KF-builder branch October 4, 2020 14:31
@onevcat onevcat restored the feature/KF-builder branch October 10, 2020 08:26
@Kaspik
Copy link

Kaspik commented Jun 14, 2021

Thanks for this PR! I have a question about migration tho - specifically about the options (not sure if I should open an issue or if this is the right thing).

Previously, we had an extension where we dynamically added options like this:

var options: KingfisherOptionsInfo = [.targetCache(ImageCache.default), .downloader(ImageDownloader.default), .backgroundDecode]
if let targetCache = resource?.targetCache.cache {
   options.append(.targetCache(targetCache))
}

if let retryStrategy = retryStrategy {
   let delayStrategy = DelayRetryStrategy(maxRetryCount: retryStrategy.maxRetryCount, retryInterval: .seconds(retryStrategy.retryInterval))
   options.append(.retryStrategy(delayStrategy))
}

if let imageProcessor = imageProcessor {
   options.append(.processor(imageProcessor.processor))
}

return Promise { seal in
   kf.setImage(with: resource?.resource,
        placeholder: placeholderImage,
        options: options,
        completionHandler: { result in
            switch result {
                case let .success(retrieveResult):
                    seal.fulfill(retrieveResult.image)
                case let .failure(error):
                    seal.reject(error)
            }
      })
}

With the new KF chaining, the options is not a thing anymore but it's a separate options chained one by one, so I wonder what would be the best way how to migrate to it? Or should we just stick with the existing approach and not migrate as you are not gonna deprecated it ever? :) Thanks!

@onevcat
Copy link
Owner Author

onevcat commented Jun 15, 2021

@Kaspik

As you said, the options are chained in this PR and you can just use it to make your code more compact and readable:

// It is a sample and maybe it won't compile
KF.url(resource?.resource)
    .placeholder(placeholderImage)
    .targetCache(resource?.targetCache.cache)
    // .downloader(.default) // The default downloader will be used even without set. So this is doing nothing.
    .retry(retry)
    .backgroundDecode()
    .setProcessor(imageProcessor.processor)
    .onSuccess{ seal.fulfill($0.image) }
    .onFailure { seal.reject($0) }
    .set(to: imageView)

I found the .retry is not receiving a nil value now, so it is a bit hard to use. I will make it receive an optional value in an upcoming release. Before that, you can make a NoRetry yourself to make it easier for nil-retryStrategy case:

struct NoRetry: RetryStrategy {
  func retry(context: RetryContext, retryHandler: @escaping (RetryDecision) -> Void) {
    retryHandler(.stop)
  }
}

I believe compared to the original style, the new one reads better.

It is totally fine to just stick with the existing approach! Adding the new KF chain-able calls only provides a new style for using Kingfisher in a good context, especially it is fully compatible with Kingfisher's SwiftUI support. That means, you can almost just copy the KF code from UIKit world to SwiftUI world, then by just changing KF to KFImage, the same magic will happen in the SwiftUI app.

There is no plan to deprecate the existing ImageView extensions methods. So just keep using it without worrying.

@Kaspik
Copy link

Kaspik commented Jun 15, 2021

Thanks! Happy to hear there is no plan to deprecate it. We will migrate some of the codebase but will leave these if-else condition-based parts as they are. :)

@onevcat
Copy link
Owner Author

onevcat commented Jun 15, 2021

No problem! I am still using a lot of the extension methods yet myself. 😂 (Too lazy to migrate to new ones all...)

skoduricg pushed a commit to rentpath/Kingfisher that referenced this pull request Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants