add support for chained requests

[pranjalsatija]

Example:

struct Person: Codable {
    let address: Address
}

struct Address: Codable {
    let street: String
}

let getPerson = PersonRequest<Person>()
let getAddress = {(person: Person) in
    return AddressRequest<Address>(person: person)
}

let street = backendService.execute(request: getPerson).onSuccess {(person) in
    return backendService.execute(request: getAddress(person))
}.onSuccess {(address) in
    return address.street
}.waitForResult()

let streetFuture = backendService.execute(request: getPerson).onSuccess {(person) in
    return backendService.execute(request: getAddress(person))
}.onSuccess {(address) in
    return address.street
}

// street is a String?
// streetFuture is a Future<String>

[CLAassistant]

All committers have signed the CLA.

[tylermilner]

Curious to hear @earlgaspard's thoughts on futures as well.

[wmcginty]

@tylermilner

I'm not sold on Future's either (mainly because of the dependency), and it may be that chaining and parallelizing requests may be something best left to the individual implementer.

I don't think the desire ever was to have this dependency part of the main spec. I'd always envisioned it as an added feature, similar to the subspecs in UtiliKit. Knowing that, I feel like idea of a subspec where X library is used to implement chaining could work, where implementers doing it their own way are free to submit a PR with a new subspec.

[pranjalsatija]

@tylermilner Thanks for the feedback! I'll push updates for those things tomorrow.

And about Futures, I don't love them either. They don't feel really Swifty to me, and I don't generally like introducing standard library style primitives that aren't actually in the standard library in any project.

However, I'm struggling to think of another way to chain requests with transformations between them. If anyone has an idea on how that could look, I'd be happy to update this PR to use that technique.

If we want to have support for multiple requests without the ability to transform values intermediately, it should be easy to make a pretty elegant solution without dependencies or any big new concepts. The same also applies for concurrent execution, which I was planning on adding tomorrow anyway.

[wmcginty]

The first thing I tried was something like:

struct ChainedRequest<T: Request, U: Request> {
  let initial: T
  let chainer: (T.ResponseType) -> U
}

This works fine for a single chain. But if you try to chain the ChainedRequest you start to run into problems because the 2nd request isn't actually created until the first network request executes (which is going to be after the chaining transformations are declared).

One possibility may be to make ChainedRequest conform to Request, but then you run into problems defining that conformance.

[pranjalsatija]

Yeah, I also tried something like that, but it gets ugly quickly, and it’s also more verbose than Futures. I think the best we can do without a dependency is adding support for chained requests without immediate transformations.

On Jan 8, 2019, 7:01 PM -0600, Will McGinty notifications@github.com, wrote:

The first thing I tried was something like: struct ChainedRequest<T: Request, U: Request> { let initial: T let chainer: (T.ResponseType) -> U } This works fine for a single chain. But if you try to chain the ChainedRequest you start to run into problems because the 2nd request isn't actually created until the first network request executes. One possibility may be to make ChainedRequest conform to Request, but then you run into problems defining that conformance. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[wmcginty]

The other way would be to basically re-implement the basic concept of Futures into Request. Because in reality, a Request<Response, Error> IS just a Future<Response, Error> with a dependency on a BackendService to execute that work.

I don't know what that looks like off the top of my head.

[codecov-io]

Codecov Report

Merging #81 into master will increase coverage by 0.12%. The diff coverage is 100%.

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage    95.6%   95.73%   +0.12%     
==========================================
  Files          35       38       +3     
  Lines        1024     1055      +31     
==========================================
+ Hits          979     1010      +31     
  Misses         45       45

Impacted Files	Coverage Δ
Tests/Helper/Mocks/MockBackendService.swift	40.74% <ø> (ø)	:arrow_up:
Tests/FutureTests.swift	100% <100%> (ø)
Tests/Helper/Result+Extensions.swift	100% <100%> (ø)
Sources/Futures/BackendService+Futures.swift	100% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d3ce16e...587e9e0. Read the comment docs.

[earlgaspard]

My thoughts on Futures coincide with other comments already stated. My main concerns are introducing a new dependency into a framework where we are trying to get dependent free as possible as mentioned by Tyler. There is also a learning curve dealing with Futures. So users of Hyperspace would not only have to learn how to Hyperspace but they would have to learn Futures too.

[pranjalsatija]

So, @tylermilner @wmcginty, how do we want to go forward with this? It seems like we're deciding between moving Future-related stuff into a subspec, dropping support for chained requests entirely, implementing some sort of ChainedRequest, or making Request behave like a Future.

My personal favorite of the above choices is making a subspec. I don't think dropping the feature entirely is a good option, I've found ChainedRequest to be cumbersome, and I'm not a fan of making Request act like a Future because the learning curve there will become even worse than it already is by having Futures.

[tylermilner]

+1 for the subspec approach. We can market chained requests as being an advanced feature that's implemented using Futures and therefore not a part of the baseline Hyperspace feature set.

[wmcginty]

That seems like a good plan @tylermilner . We would need to remove the execute function returning a Future from the Request protocol in order to separate out into a subspec, but I don't think that's an issue as there is only really one way to implement that functionality. Everything else is already in extensions and only depending on existing BackendService/Request functionality.

[wmcginty]

What is going on with Codebeat? I can't seem to find a way to force it to re-run its analysis.

[tylermilner]

Maybe if we push another commit, it will re-trigger the codebeat analysis?

[wmcginty]

That's the best answer I can come up with. Quick, somebody find a typo!

[pranjalsatija]

@tylermilner @wmcginty 🎉