Separate "Request" and "Task" APIs?

[nonsensery]

tl;dr

Maybe the ServiceTask request-building methods should be separated from the response-processing methods.

Discussion

The ServiceTask is used both for setting up a request, and for pausing/resuming/processing the response. In practice, it's not very common to want to expose both APIs. It can lead to confusing situations like this:

class BrewClientAPI {
    func fetchAllBrewers() -> ServiceTask {
        return webService
            .GET("/brews")
            .setHeaders(someHeaders)
            .resume()
    }
}

// Elsewhere:

brewClientAPI.fetchAllBrewers()
    .setHeaderValue(specialAuthSecret, forName: SpecialAuthHeader) // <-- Ruh roh!
    .updateUI { /* and so on… */ }

The "special auth header" will not be sent to the server because a snapshot of the request was created when resume() was called. But the calling code has no good indication that the request can no longer be modified.

Here's another one:

class BrewClientAPI {
    func fetchAllBrewers() -> ServiceTask {
        return webService.GET("/brews") // <-- Oops, forgot to call resume()!
    }
}

// Elsewhere:

brewClientAPI.fetchAllBrewers().updateUI { /* and so on… */ }

This service call is never sent and there's no obvious indication why.

Proposed Changes

As part of the breaking changes for version 4, I'd like to propose removing the request-building methods from ServiceTask. Since these methods all just call through to the underlying Request instance, we could just expose the request directly instead. The API changes might look something like this:

extension WebService {
    func GET(path: String) -> Request {
        return Request(method, url: absoluteURLString(path))
    }
}

struct Request {
    func send(session: Session) -> ServiceTask {
        return ServiceTask(request: self, session: session)
    }
}

And here's a usage example:

class BrewClientAPI {
    func fetchAllBrewers() -> ServiceTask {
        return webService
            .GET("/brews")
            .setHeaders(someHeaders)
            .send(webService)
    }
}

// Elsewhere:

brewClientAPI.fetchAllBrewers()
    .setHeaderValue(specialAuthSecret, forName: SpecialAuthHeader) // <-- compiler error, no such method
    .updateUI { /* and so on… */ }

And another:

class BrewClientAPI {
    func fetchAllBrewers() -> ServiceTask {
        return webService.GET("/brews") // <-- compiler error, cannot convert Request to ServiceTask
    }
}

Unanswered questions

I'm not sure how the passthrough delegate should be assigned in this scenario. One thought is that the ServiceTask could check whether its session is a WebService, and actively pull the delegate from it.

I don't love the session parameter to send(), although it's also kind of cool how it allows you to save off a request and send it multiple times (if that's something you're in to).

[angelodipaolo]

👍 👍

I am very much in favor splitting up ServiceTask from Request and have been wanting to propose a change like this with the intention of introducing it into v4.

This is how I've envisioned the changes:

• Remove request building API from ServiceTask.
• Make Request's initializer public so that requests can be constructed independently of both WebService and ServiceTask
• Change the request API of WebService (ie .GET(path:) and its variants) to return a Request value that has been initialized with a URL relative to WebService's baseURL configuration instead of returning a ServiceTask.
• Add a method to WebService for creating a ServiceTask from a Request, something like: func serviceTask(request:) -> ServiceTask.

With the Request initializer made public, a Request struct can be directly created and mutated.

// Build a request independently of the ServiceTask chain
var request = Request(.POST, url: "http://httpbin.org/")
request.formParameters = ["foo": "bar"]

// Create ServiceTask from request
service
  .serviceTask(request: request).
  .response { data, response in

  }
  .resume()

Building a request off of WebService would give you a request that is based on the baseURL of WebService

// Build a request based on the `WebService` baseURL configuration
var request = service.POST("foo")
request.formParameters = ["foo": "bar"]

// Create ServiceTask from request
service
  .serviceTask(request: request).
  .response { data, response in

  }
  .resume()

The chaining methods could be moved to Request although I am in favor of eventually removing them entirely.

// Build a request independently of the ServiceTask chain

var request = service
                  .POST("foo")
                  .setFormParameters(["foo": "bar"])


// Create ServiceTask from request
service
  .serviceTask(request: request).
  .response { data, response in

  }
  .resume()

Of course this means that syntax like this will no longer be possible but this should be considered an improvement for the sake of clarity and safer code:

service
  .POST("foo")
  .setFormParameters(["foo": "bar"])
  .response { data, response in

  }
  .resume()

We could retain the chaining behavior from request-to-response if we added a method to Request that returns a ServiceTask, like your send() example:

service
 .POST("foo")
  .setFormParameters(["foo": "bar"])
  .send()
  .response { data, response in

  }
  .resume()

But reading this code is a little odd because send() isn't actually sending anything. The request is actually sent the first time resume() is called and I'd like to stick to the resume/cancel/suspend API in order to be consistent with NSURLSession and friends. A method like send() would be misleading next to the other lifecycle methods.

It is worth noting that the original API for building requests was not well-received and there was a lot of Slack discussion to change it that ended with the consensus favoring the chaining style for request building. Also worth noting is that this setter chaining style is called out as something that should be avoided in the best practices guide that we forked into Electrode-iOS.

Since this has been a debated subject in the past, I'd like to get some feedback from others before we make a decision on what the API should look like to separate ServiceTask from Request but I definitely want to see this happen for v4.

[nonsensery]

Glad to hear you feel the same way. There's one thing that concerns me about this pattern:

service
  .serviceTask(request: request)
  .response { data, response in /* snip */ }
  .resume()

Which is that there is no help from the compiler to make sure that you call resume(). If you don't call it, the task just sits there waiting.

Thinking about it, I'm not sure that resume() is even useful. AFAICT, it's purpose is to allow you to configure the request before sending it. But with the new API, that will be done by mutating the request before passing it to serviceTask().