onevcat / Kingfisher

A lightweight, pure-Swift library for downloading and caching images from the web.
MIT License
23.41k stars 2.66k forks source link

`defaultOptions` only invokes _last_ `requestModifier` in chain #2210

Open nodediggity opened 9 months ago

nodediggity commented 9 months ago

Check List

Thanks for considering to open an issue. Before you submit your issue, please confirm these boxes are checked.

Issue Description

What

I am trying to chain multiple requestModifier instances within a SwiftUI app. In my "real world" example, each modifier serves a different purpose and contains conditional logic that either modifiers the request or allows it to pass through unchanged.

To avoid complexity in our views I am using KingfisherManager.shared.defaultOptions to configure this away from my SwiftUI views.

It is my understanding setting a value here will provide this behaviour on a global level - all instances of KFImage will inherit these options.

final class FirstRequestModifier: ImageDownloadRequestModifier {
    func modified(for request: URLRequest) -> URLRequest? {
        print("^^ FirstRequestModifier")
        return request
    }
}

final class SecondRequestModifier: ImageDownloadRequestModifier {
    func modified(for request: URLRequest) -> URLRequest? {
        print("^^ SecondRequestModifier")
        return request
    }
}

KingfisherManager.shared.defaultOptions = [
    .requestModifier(FirstRequestModifier()),        
    .requestModifier(SecondRequestModifier())
]

I also understood that modifiers are invoked in the order in which they are appear in the options array.

When running my app however I am finding that only the last option in the array is invoked.

Any modifiers previous to this are either overwritten or ignored.

Reproduce

Attached is a basic example that replicates this.

Each modifier should log to the console in the order they are created.

KingfisherExample.zip

Other Comment

I am using v7.11.0 and supporting iOS 17.

onevcat commented 9 months ago

Currently the requestModifier option only keeps the last value, it is not linkable and cannot be called in a chain.

Instead of setting multiple . requestModifier, you need to create a "multiple version" of ImageDownloadRequestModifier yourself, like this:

final class ConcatenatedRequestModifier: ImageDownloadRequestModifier {
    let modifiers: [any ImageDownloadRequestModifier]
    init(modifiers: [any ImageDownloadRequestModifier]) {
        self.modifiers = modifiers
    }

    func modified(for request: URLRequest) -> URLRequest? {
        var r = request
        for modifier in modifiers {
            if let modified = modifier.modified(for: r) {
                r = modified
            } else {
                return nil
            }
        }
        return r
    }   
}

Indeed it is a good idea to allow the option accepts multiple .requestModifier as a chained behavior. However, it breaks the current behavior and usage (although this should be a rare use case). I will consider it in the following major upgrade!

Thanks for the suggestion.

nodediggity commented 9 months ago

Ah thank you so much, I misunderstood how it works, your suggestion worked perfectly.

Feel free to close this, I really appreciate the speedy response 🙏