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

Consider simplifying usage of 3rd-party Futures libraries with Carlos #120

Closed
MaxDesiatov opened this issue Jan 25, 2016 · 7 comments
Closed

Comments

@MaxDesiatov
Copy link

I've started using https://github.com/Thomvis/BrightFutures in my project as Carlos' Future implementation is not powerful enough: no possibility of chaining and thus no easy interaction with other async code compared to what 3rd-party libraries provide.

Currently the most popular and clean library in my opinion is BrightFutures, which I've picked for my project. The main problem is that both Carlos and BrightFutures provide a public Future symbol which doesn't exactly clash when imported in the same file, but doesn't resolve smartly either. BrightFuture's Future has two generic parameters while Carlos' Future has only one. Still, when writing a type declaration like Future<[CKRecordID], NSError> Swift compiler for some reason expects it to be the one-parameter generic version from Carlos. This leads to a redundant type specifications in my code like BrightFutures.Future<[CKRecordID], NSError> in files where BrightFutures and Carlos are both imported.

Thus I see three possible solutions to this:

  1. Future class in Carlos is renamed to something more specific like CFuture or whatever.
  2. Carlos public APIs could define a protocol with an unambiguous name like FutureType and by default return it's own implementation of the protocol, which would be hidden from the user though. If user would want to override the return type, there would be an override point provided as a metatype property or a block property, whichever is considered more convenient.
  3. Carlos uses a 3rd-party Futures implementation, which would be more powerful than the current one, or the internal implementation could be extended to be more powerful, requiring more maintenance though.

I hope that @vittoriom considers option 3 with BrightFutures as it seems to me so far the most stable, powerful and thoroughly documented library. Also, cases like batch get/set could be generalised with code like this:

["a", "b"].traverse {
    cache.get($0)
}.onSuccess { result in
    // result is an array of OutputTypes
}

Obviously, I don't know the original motivation of Carlos implementing it's own Futures library, so please clarify if some of the suggestions don't make sense.

@vittoriom
Copy link
Contributor

Hi @explicitcall, and thanks again for contributing to Carlos with your observations!
I agree that Carlos' Futures are not nearly as powerful as the ones from BrightFutures, also considering that the latter is heavily based on providing Futures and Promises and such more focused on this specific task.

Even though it's not my goal to make you use Carlos' Futures instead, I'd just like you to know that async composition of Futures is indeed already possible, just marked as internal until everything is properly tested. I expect Futures' composition and nice support for GCD tasks composition too will be added in the 0.7 release.

Back to your main issue, that is making BrightFutures' and Carlos' Futures coexist:
Given that Swift namespaces solve this specific issue (I didn't investigate how the same symbol is resolved when multiple namespaces provide it - maybe reordering the imports gives more priority to one over the other? - I agree that the solution is not optimal, but temporary.

Of the options you provided, I would skip the first since it implies changing the name of a symbol just because somebody may also use another Future library; I like the second one, and I would indeed use it if it was not for the issue that generic protocols have to rely on typealiases that cannot be easily specified as return types in generic functions. I tried to solve this issue before and didn't manage to (this is also one of the reasons why the return type of many APIs is BasicCache instead of CacheLevel), so unfortunately for me this is not a viable option.

The third option is on a first look the best to me too. The only thing that keeps me from going for it is that it would force people who want to use Carlos to also use BrightFutures as a dependency (also, we have to consider all possible installation methods of Carlos: CocoaPods, Carthage, simple download of the Github repo, git clone. Introducing a dependency would be more or less easy depending on the method used). I would like to do my best before having to bring a dependency together with the project, and even though I can't exclude we will indeed include BrightFutures (or a similar library) into Carlos at some point since we also make heavy usage of async computation in general, I don't feel totally comfortable doing it now, with the not-too-heavy requirements we currently have.

My proposal would then be: what would you like to see in Carlos' Futures that is not present right now? What are some necessary requirements that you, as a Futures user, are missing in our implementation?

@MaxDesiatov
Copy link
Author

For me the main benefit of using Futures is an option to avoid the notorious "Pyramid of Doom" and instead of

futureReturningFunction().onSuccess {
    cache.get($0).onSuccess {
        cache.set($0).onSuccess {
            anotherFutureReturningFunction($0)
        }
    }
}

I would like to write

futureReturningFunction()
    .flatMap(cache.get)
    .flatMap(cache.set)
    .flatMap(anotherFutureReturningFunction)
    .onFailure {
        ...
    }

As a nice bonus onFailure is invoked when either of async computations failed with no need to specify error handling closure for every future.

Also, BrightFutures provides all familiar SequenceType abstractions like map, fold, zip, filter etc that are great time-savers when dealing with collections of futures, which allow to get rid of special cases for batch get/set.

As far as I'm aware after looking at documentation and source code none of this is applicable to Futures that get/set return in Carlos.

@vittoriom
Copy link
Contributor

You're totally right. I have to seriously think whether to put some more functional love to Carlos' Futures or to find a good way to include BrightFutures without having too many disadvantages when it comes to cloning the repo from scratch or actually including Carlos as a dependency in a project.

Thanks again for putting some thought into this!

@MaxDesiatov
Copy link
Author

Personally as a Carlos library user I'm totally fine with another dependency as both Cocoapods and Carthage handle subdependencies really well. As for repository clones, I would recommend using either a submodule (which is a bit fiddly but does the job) or copying in a snapshot of BrightFutures, but not including that snapshot in .podspec source list. I think there also should be a way to keep a snapshot copy with Carthage (maybe creating a separate target for Carthage integrations that doesn't use the snapshot)

@vittoriom
Copy link
Contributor

As a temporary workaround - but actually something that could work as a Carlos extension in case I would proceed with enhancing Carlos' Futures without integrating BrightFutures into the project - you could create an extension of Carlos' Future that adds a getter property returning a BrightFuture instance instead. This way, you can actually use the whole power of BrightFutures without changing Carlos at all. Just convert one Future into the other and then call flatMap or whatever else is needed.
What do you think?

@MaxDesiatov
Copy link
Author

Turns out

typealias BFuture = BrightFutures.Future
typealias BPromise = BrightFutures.Promise

works really well for me in files where I import both BrightFutures and Carlos. In other cases I can write wrappers and that works fine as well. I think this ticket can be closed.

@vittoriom
Copy link
Contributor

Glad to know typealiasing worked out!
I'll create an issue to enhance Carlos' Futures anyway, and close this one.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants