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

Suggestion: AllSettled implementation #70

Open
hannessolo opened this issue May 12, 2020 · 5 comments
Open

Suggestion: AllSettled implementation #70

hannessolo opened this issue May 12, 2020 · 5 comments

Comments

@hannessolo
Copy link

Hi,

I've been using your library, and it's been fantastic so far. However, recently I came across a use case where I needed something like allSettled from JavaScript.

I came up with the following extension:

extension Promises {
    public static func allSettled<T>(_ promises: [Promise<T>]) -> Promise<[T?]> {
        return Promises.all(promises.map { promise in
            return Promise { fulfill, reject in
                promise.then { value in
                    fulfill(value)
                }.catch { _ in
                    fulfill(nil)
                }
            }
        })
    }
}

This uses optional types to resolve an array of promises to either their value, or nil if the promise fails.

Use Case

This can be useful if we need to wait for many actions to complete but don't care if some fail.

For instance, let's say we are loading 100 images for a social media feed. If some of the images fail to load, we don't want the entire operation to fail - we would rather display the images that did load, and place some placeholder for the ones that failed. This can easily be done with optional unwrapping, letting us deal with the error handling at a later stage.

The extension I wrote works well for me, but it would be nice to have something like this in the official API - my code is probably far from optimal anyway 😊.

@khanlou
Copy link
Owner

khanlou commented May 12, 2020

I'm not opposed to this as an additon per se, but two thoughts here:

First: ideally, I'd like to be able to express this with something like this:

let ignoringFailures = Promises.all(promises.map { promise in
    return promise.recover({ _ in nil })
})

But we can't quite write that. Combine has .replaceError(with: nil), so maybe we could pull some inspiration from there?

Second, if you're loading 100 images for a feed, why do you need to know that all of them are done (or fails)? You definitely want to know when each of them is done, so you can update the corresponding cache or image view, but what work are you doing when they all settle?

If there's anyone else who wants something like this, feel free to chime in with some use cases and we can discuss!

@hannessolo
Copy link
Author

Thanks for your response.

About the use case: You're right, maybe that was not the best example. In my use case, I'm essentially loading a bunch of "widgets" (between 5 and 10) that the user configured remotely. All of the widgets must have finished loading before we actually show the screen that presents them (this, as you say, is different for a social media feed where we can also load images individually on demand). If one request fails, I don't want to cancel the entire app from loading.

My code is similar to this (slightly shortened):

// Load config telling us which widgets the user wants
widgetLoader.getConfig()
.then { configuredWidgets in
     // Load the widgets
     let futureWidgets = self.load(configuredWidgets)
     return Promises.allSettled(futureWidgets)
}.then { loadedWidgets in
     // Add loaded widgets to the UI      
    for widget in loadedWidgets {
          guard let widget = widget else { continue }
          self.widgets.append(widget)
     }
     notifyDoneLoading()
}

Now I think about it, it is actually a relatively niche use case. However, what prompted me to open this issue is that JavaScript recently added allSettled, so I imagine it must be at least a semi-frequent use case.

For your second point, I agree. However it got me thinking that one of the selling points of this library is its small size, so I'm not sure whether adding such a feature is worth it. In that case, maybe it would also be better not to add allSettled. What do you think?

@khanlou
Copy link
Owner

khanlou commented May 14, 2020

Ah, that use case makes a lot of sense.

I think for this, I'd like to leave this issue open for a little while and see if anyone else has any feelings on it. My current feeling is that is a bit niche, but I'm hoping to hear from a few other people and their use cases and we can make a call then. How does that sound?

@hannessolo
Copy link
Author

I agree - leaving it open for a while sounds like a good plan.

@teawithfruit
Copy link

I have a simmilar use case.
I've got some Alamofire requests in a chain. It is possible, that some of them returns with an 400 status code. If this happens, the rest of chain should'nt be rejected.
In this case an allSettled function would be very helpfull.

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

No branches or pull requests

3 participants