Fuel: Refactor Fuel to make it more extensible

[asarkar]

I really like Fuel but I think the existing design doesn't allow for easy extension. Here're some design suggestions that, IMO, will make the library a lot more user friendly and also reduce code. Thoughts?

1. Users should be able to use any encoder, decoder, HTTP client they want using the builder pattern on Fuel, like so:

Fuel.builder.build() // creates Fuel with all default settings
Fuel.builder
  .encoder(FuelJacksonEncoder())
  .build() // creates Fuel with Jackson encoder and rest default
Fuel.builder
  .encoder(FuelJacksonEncoder())
  .client(FuelOkHttpClient())
  .build() // you get the idea

Each of the components FuelJacksonEncoder etc also follows the same builder pattern.

1. Do away with the String.* extension methods. You always start with a Fuel instance as shown above, plain and simple.

2. The Triple is kind of confusing, because the Result is sort of forced on the user. When I make a HTTP request, I expect a response, and thank you for also providing the request at the same time. If I didn't ask you for a Result object, don't force it on me.

fuel.get("http://httpbin.org/get")
  .response { request, response ->
    // response.body would be Array<Byte>
    // response.bodyAs<T> would use the decoder to convert to T, where T can be, but not limited to, InputStream
    // response.as<Result> would convert the response to a Result object provided result module is in compile scope. If not this code wouldn't compile.
  }

1. Do away with responseString and responseJson as point 3 above is enough.

[kittinunf]

Hey, thank you @asarkar for your opinion, it is really helpful.

1.

One thing that I wanna set Fuel apart from a popular library like Retrofit is to offer the ease of usage. Having some sort of builder or configuration class seems to go a bit different than what I have in mind for Fuel. I know, this is debatable (because as of now Fuel already has FuelManager which is still a configuration), but it was designed from the beginning to be like that.
Speaking for Jackson module, I, at least personally, want it to be just another set of deserializer like what we have already done for Gson.

2.

I found the String extension to be very helpful when trying things out quickly like calling some other full URL that differs from your regular one. I have discussed with a lot of people and some of them also expressed the same feeling as you as well where they do not like those String extension. However, I don't have a strong reason to remove them either. I am not very adamant on this one though. I think if we can find a good reason to deprecate them, I think it is acceptable.

3.

I mostly agree with you on this one. I feel that originally 2 years ago, I designed API to be super simple and prevent users to memorize anything. So, I figured out that I could return everything (request, response, result) so in the case, you don't need them just ignore (with _ in Kotlin), and when you do just use it. After I have used it myself personally, I think I have only remembered just a handful of occasions that I need to do something with Request. Meanwhile, I think Response is still helpful though because I have numerous of occasions that I have to check for response's status code.

So, you ask me in 2017, I might actually want to change the API to something like this instead

Fuel.request("https://httpbin.org/get").response[String|Json|Object] { response ->
}

Then, if there is a need to access for request or result. We might expose to the user like this

//access result
when (response.result) {
  is Success ->
  is Failure ->
}

//access request
println("Request: ${response.request}")

I believe (in a good intention) that Result object that I expose to the user is a good abstraction (maybe I am too optimistic). Also, it dictates the type T in result base on the interface that user call either (String/Json/Object). So, I feel like they are still beneficiary to the user.

But I am very open for response.dataStream to open for the user to use for any other purpose as they see fit.

I hope I share some of my thoughts in here, what do you think?

[asarkar]

@kittinunf Here is my 2c.

...offer the ease of usage. Having some sort of builder or configuration class seems to go a bit different...

I understand where you are coming from, but I think sooner or later, you'll start seeing more requests for customization, especially as more users pick up on Fuel in specific, and Kotlin in general. Builder is a well-known pattern, and like you said, Fuel already has one, so having a single and consistent way of creating a client doesn't seem very complicated to me.

String extension

I made the case for removing them to simplify the API, and have a consistent workflow. If starting from String, it's not unreasonable to start from a URL, or URI, which you don't currently support. Why not leave all that conversion to the user instead of increasing the bulk of the library?

Result

I was arguing for removing it because it brings in a dependency on Result, as part of fuel core, and you're making a big assumption that the user even wants to use a functional abstraction.

As for response.request, I don't like that abstraction much; a response does not contain a request, it is generated because of a request (cause and effect, happens after). If you happen to change it, I'd rather have a .response { request, response -> over response.request, or even .response { response ->.

[kittinunf]

@asarkar Thank you for the discussion. To move this forward, do you think would it be nice to start from the things that we pretty much on the same page. Maybe starting from 3. and we are moving on from there.

[yoavst]

1. I think FuelManager is OK for configuration and there is no need for extra builder.

FuelManager().apply {
client = FuelOkHttpClient()
encoder = FuelJacksonEncoder()
}

However, I think we should create a Fuel class that is created from FuelManager and pass the requests to the manager class:

val fuel = Fuel.of(fuelManager)
fuel.get("https://www.google.com")

The Fuel class will have a companion object that extends Fuel and has a property manager: FuelManager. In this way normal users could use Fuel as before.

1. The string extension are indeed very useful for quick prototyping. I use them a lot. We can move them from the main module to fuel-string-extensions module. A support for URL and URI and android Uri is nice, but seems like duplication. That's why I think the extraction to a different, extra, module is good - since it is extra we don't need to support everything.

2. The response[String|Json|Object] is OK in my opinion. Just an idea that will probably be good enough for you two (but will be ~magic):

data class ResponseResult<K>(val request: Request, val response: Response, val result: Result<K, FuelError>)
fun Request.response[String|Json|Object](callback: ResponseResult<[String|Json|Object]>.() -> Unit)

So if you don't need to request you can forget about it.

1. I think Fuel is hold back by Java support. Many methods that could have been extension methods are normal methods for Java support (Like response[String|Json|Object] in Request class). Cleaning the classes and putting extra functionality in extensions is a great improvement imo for the code design.
   Java support is great feature, but do we really need it?

[asarkar]

@yoavst What's the difference between Fuel and FuelManager? What roles do they play in the design?

As for Java support, I think it's fair to say that there are many good libraries, and I don't really see people reaching out to a Kotlin library from Java for making HTTP calls. But then again, extension methods are cool, but when to use them and when not to? Thank God, Kotlin extension methods are not like Scala implicits, which is my biggest pet peeve against Scala; forgetting an import could break your application or worse, change the behavior.

[yoavst]

FuelManager is like the builder you suggested. It creates the configuration for creating a request. Fuel is just a simple interface to call it with simple API.

About extension, this is how I see it:
You provide the basic, minimal working functionality in the class itself, and enhance it with extension methods. Take a look at the methods response[String|Json|Object]. Are they integral part of the Request class? They are just wrapper over response that apply a deserializer. They should not be part of the binary interface of Request, which should be as minimal as possible.

Actually, as I see it. Request should be a POJO with no methods. All the builder functionality of request is simply extension and should be defined as such. But again, it is open to discussion and I'd like to hear your opinion about it.

In addition to moving the builder methods to be extension, I would like the make Request and Response a data class like structure. We can pass all the implementation details of Request to RequestBuilder or RequestImpl, and return Request on the callback (since all the extra parameters belong to creating the request internally, and depends on the platform - jvm vs js vs native.

[yoavst]

Seems like we didn't reach an agreement, so let's go with baby steps.
Do we have an agreement about making Request a dumb class and moving all the network related config to somewhere else? In this way we can pass Requests to different Fuel instances and have different configuration for each instance (it also means that all the base properties will be added only on request). No binary/source change should be required, since it will modify only internal and private fields.

[asarkar]

@kittinunf and I had agreed that the response signature is unwieldy. I didn't get time to look into the source code, so I didn't comment further. As a result (of not having looked into source code), I only partly understand @yoavst proposal regarding refactoring Request. Perhaps, @kittinunf can comment on that, or you can just submit a PR with what you have in mind.

[yoavst]

My Idea:

FuelBuilder / FuelManager:

• Internal configuration for request (same as now. but if you want to rename it I'm not against it).

Fuel class + Fuel companion object:

• Class that internally calls FuelManager and provides more friendly API for request.
• One can build Fuel instance from provided FuelManager.
• For backward compatibility and for simple cases, Fuel has companion object: Fuel that has FuelManager mutable field. So you could just call Fuel.get(...) if you need only a single Fuel instance.

val fuel = Fuel.of(fuelManager)
fuel.get("https://google.com").responseString() // custom fuelManager
Fuel.get("https://google.com").responseString() // default/singleton fuelManager

Request + MutableRequest:

• A Pojo class for creating a request.
• Does not contain any implementation detail (like Future, SSLSocketFactory...).
• Instead, FuelManager config (headers, parameters, ssl,...) will be injected on Client.
• We do need to find a way to pass the configuration (FuelManager). Maybe will have a internal field with FuelManager instance, and a public method on Fuel class fun apply(request: Request): MutableRequest (which will replace the instance).
• We can also provide MutableRequest interface which you receive from Fuel.method(...) and is mutable. the Request you receive from responseX {} is immutable.
• All the mutating methods of MutableRequest will become extension methods, though they will still be the preferred way to mutate a request.
• One can edit MutableRequest as builder:

MutableRequest.apply {
 url = "https://www.google.com"
 parameters += "hello" to "world"
 headers += "user-agent" to "Fuel"
}

Request flow:

1. Create FuelManager instance.
2. Create Fuel instance from (1).
3. Call fuel.method() on (2) instance to get MutableRequest instance and mutate it.
4. Call responseX() on (3) instance.
5. A class will merge FuelManager instance (1) and the MutableRequest (3) to a single Request.
6. The request instance (5) will be passed to Client of (1) for executing.

Javascript support #134:
Now it will be really easy to support Javascript.

1. Make FuelManager an header class.
2. Provide different impl classes for each platform.
3. Provide different default Client for each platform.
4. Classes with JVM features like ResponseDeserializable can also be different on each platform. Environment can be changed from callbackExecutor: Executor to fun callCallback(callback).

Should work 👋

[dpmaycry]

So

val fuel = Fuel.of(fuelManager)
fuel.get("https://google.com").responseString() // custom fuelManager
Fuel.get("https://google.com").responseString() // default/singleton fuelManager

Can we use it now?

Or to say, can we create Fuel from FuelManger()

[SleeplessByte]

val manager = FuelManager()
manager.get("https://google.com").responseString()

Works now. Let's close this and re-open separate tickets if we want to implement https://github.com/kittinunf/Fuel/issues/225#issuecomment-328327731