search

haf / Http.fs

A simple, functional HTTP client library for F#
322 stars 43 forks source link

Have a common API between Http.fs and Thot.Http #142

Open MangelMaxime opened 6 years ago

MangelMaxime commented 6 years ago

Hello guys,

in the past week I implemented an equivalent of Http.fs for Fable. This implementation is a port inspired by Elm and elm-http-builder.

Example of the Api:

// Query an url and expect a string as response
let request =
    Http.get "http://localhost:3000/posts/1"
    |> Http.withExpect Http.expectString

// Query with queryParams, a cacheBuster
let request =
    Http.get "http://localhost:3000/posts/1"
    |> Http.withQueryParams
        [ "firstname", "maxime"
          "surname", "mangel" ]
    |> Http.withCacheBuster "cacheBuster"
    |> Http.withExpect (Http.expectStringResponse (fun response -> Ok response.Url ))

As you can see the philosophy, is similar with Http.fs.

I propose to provide the same Api for the request builder in Thot.Http. This should make it possible to share Request code between the server and the client.

The common code would be the request builder and each platform would have it's own runner.

Are you ok with this vision ? And so ok if I take part of the Http.fs code ?

haf commented 6 years ago

I suggest it's colocated with http.fs to avoid divergence over time and that we cooperate on this repository without further name changes in that case. What do you think?

MangelMaxime commented 6 years ago

Sorry it is a long message and I hope it's understandable.

I will try to explain what is needed if we colocate the codes, the vision of Thot, and the difference I saw between both API.

Some thinking

Hosting the code on the same repo is doable, but some infos need to be considered.

Benefit from hosting it on Thot repo. It goes along my vision of Thot:

Thot is a set of several libraries for working with Fable applications.

It's like a toolbox for Fable applications.

About the name

Because we will need to publish 2 differents packages, we will need a new name for the Fable version. (I would like to keep Thot.Http :D but open to change).

API difference

Some difference between Http.fs and Thot.Http:

      /// If set to true, then the cookies are send with the request
      WithCredentials : bool

This is a limitation compare to Http.fs

      /// Function used to describe the result we expect
      Expect : Expect<'Out>

This is an addition compare to Http.fs, probably consider like a major one

      CacheBuster : string option
      /// Query params to add to the request

This is an addition compared to Http.fs, but really small

type HttpError =
    /// The url you provided isn't valid
    | BadUrl of string
    /// The request took too long to execute.
    | Timeout
    /// We couldn't reach the server. Expect, wifi was turn off, the server was down, etc.
    | NetworkError
    /// You got a response but the response code indictact an error.
    | BadStatus of Response
    /// You got a response with a good response code. However, the body of the response was something unexpected.
    /// Example: The `decoder` failed.
    | BadPayload of string * Response

This point probably doesn't matter as it depends on the runner used.

We can have a different way to send and receive a request depending on the target as it's dependant on the runtime. The send/receive part is not concern by what I called "shared code".

For me the shared code is only about the Request builder.

Please feel free to ask more info if needed :)

haf commented 6 years ago

We will need to publish two differents packages in order to not include Hopac and System.Diagnostics.FileVersionInfo dependencies on Fable apps.

I suggest HttpFS.Core with the object model used by both and a HttpFS.Net referencing Hopac and compiling on .Net Core and net461.

That way the model can be reffed anywhere and is pure F#.

Your HttpError type is already enforced throughout Http.fs; it does not throw exceptions. I'm very open to unifying this API.

The cache buster is a non HTTP thing for broken web sites and apps. I'm not very keen on it as saving files with their names = their hashes is a better and more efficient way of doing the same.

Cookies could be moved to HttpFS.Net, or we could interface the cookie module in the browser.

The code you've written could go in HttpFS.Browser (for XMLHttpRequest usages (or fetch))

What's Expect for? Would that not be better in a filter (functor)?

MangelMaxime commented 6 years ago

I am fine with using 3 packages 👍

Your HttpError type is already enforced throughout Http.fs; it does not throw exceptions. I'm very open to unifying this API.

Can you please point me to the code ? Seems like I can't find it.

The cache buster is a non HTTP thing for broken web sites and apps.

I am ok to remove it, I just added it because it was present in elm-http-builder package. If users need to it should be easy for them to add it.

Cookies could be moved to HttpFS.Net, or we could interface the cookie module in the browser.

I was thinking to detect when users try to set cookies and so set the flag to true in the XMLHttpRequest.

Another solution can be to use conditionals:

#if FABLE_COMPILER
....
#else
...
#end

The code you've written could go in HttpFS.Browser (for XMLHttpRequest usages (or fetch))

I wrote Thot.Http to not use the fetch Api provided by Fable.Powerpack. Because, this is an implementation detail hidden from the end user I feel like keeping XMLHttpRequest.

The reason, being I was able to capture all the possible error and map them nicecly to my HttpError type.

What's Expect for? Would that not be better in a filter (functor)?

I don't know the functor name.

Quoting Elm documentation Expect is:

Logic for interpreting a response body.

For example, if you are waiting a JSON value, you can provide a "json decoder" (logic to deserialize into a type). This decoder, will be directly applied to the request body.

With the current implementation of Thot.Http it's applied automatically when the body is received.

let private handleResponse (xhr : Browser.XMLHttpRequest) (responseToResult : Expect<'Out>) : Result<'Out, Error> =
    let response : Response = toResponse xhr

    if response.Status.Code < 200 || response.Status.Code >= 300 then
        // Here we know that response if of type Response<string>
        // Because it's coming directly from the xhr answer, we didn't apply any transformation to it yet
        response |> BadStatus |> Error
    else
        match responseToResult.ResponseToResult response with
        | Ok result ->
            Ok result
        | Error msg ->
            BadPayload (msg, response) |> Error

We could rewrite it like:

let private handleResponse (xhr : Browser.XMLHttpRequest) : Result<Response, Error> =
    let response : Response = toResponse xhr

    if response.Status.Code < 200 || response.Status.Code >= 300 then
        // Here we know that response if of type Response<string>
        // Because it's coming directly from the xhr answer, we didn't apply any transformation to it yet
        response |> BadStatus |> Error
    else
        Ok response

And provide function to handle the Response like apply a decoder, return the body as a string only, etc.

haf commented 6 years ago

point me to the code https://github.com/haf/Http.fs/blob/master/HttpFs/HttpFs.fs#L797

I am ok to remove it, I just added it because it was present in elm-http-builder package. If users need to it should be easy for them to add it.

Could we not build a collection is nice-to-have-filters? I've started at the bottom of the file. A cache-busting query string thingy should be just that.

detect cookies

Seems like a slightly leaky abstraction since cookies are not set via your code in the browser.

To be honest I've never found any usages for that boolean, since default is to send the cookies the browsers has. Have you ever used it?

I wrote Thot.Http to not use the fetch Api provided by Fable.Powerpack. Because, this is an implementation detail hidden from the end user I feel like keeping XMLHttpRequest.

Same here, I like to be able to cancel my requests, via RxJS.

I don't know the functor name.

Functor is a mapping between two functions. You can see it as a function that takes a function as input and changes how it works. So a functor could be of signature (Request -> Alt<Response>) -> Request -> Alt<Response>; as denoted by the parenthesis, it acts on the 'send' function. You can find more examples in the Fakta PRs and logary-js and at the bottom of HttpFS.fs https://github.com/haf/Http.fs/blob/master/HttpFs/HttpFs.fs#L1102

What your Expect is, is really just a functor, like above, that interprets the response, except that it's less general. I'm happy to give you more pointers, as I feel there's a good reason to provide end users with the strongest abstraction beneath a friendly compositional interface, rather than a friendly weak abstraction at the core and half-measures on top.

Here is your code written as a filter:

let private handleResponse: Filter<_> =
  fun next (xhr: Browser.XMLHttpRequest) ->
    let response : Alt<Response> = next xhr
    response |> Alt.afterFun (fun response ->
    if response.Status.Code < 200 || response.Status.Code >= 300 then
        // Here we know that response if of type Response<string>
        // Because it's coming directly from the xhr answer, we didn't apply any transformation to it yet
        response |> BadStatus |> Error
    else
        Ok response)
MangelMaxime commented 6 years ago

Could we not build a collection is nice-to-have-filters?

Sure we can add withCacheBuster (name for example) to the collection and it's more or less the goal of the request builder.

Functor is a mapping between two functions.

I am totally fine for using this concept.

Are you ok, if I start working on what we discussed ? Like splitting the repo etc.

haf commented 6 years ago

Do we have to split the repo for this? Can't I give you complete access to this repo?

haf commented 6 years ago

https://github.com/haf/Http.fs/projects/1 has a project for this, too ;) Not the server-parts of course, but the object model parts.

haf commented 6 years ago

Basically, I don't like churn too much (and I like stable APIs)... I have a lot of things depending on this code base with this name. Besides, Http.fs is more well known than Thot, and Thot pronounces tott in Swedish which is a skiing hotel.

Also the with... functions have been lessened a bit in v5 that is the current code-base, because having with- as a prefix to all functions didn't actually add any value.

haf commented 6 years ago

So yeah, I'm OK with the gist of it!

MangelMaxime commented 6 years ago

Sorry @haf , I said "Like splitting the repo" when meaning "Like splitting the package / code"

And I am ok for the with... part, it was just an example. My idea, is to split the code (between HttpFs.Core and HttpFs.Net).

Then when everything is green again, start working on implementing HttpFs.Browser, with current API. And when doing it, note my remark and discuss it with you and the maintainers.

MangelMaxime commented 6 years ago

If you want to give me access to this repo, I will be able to work in a separate branch instead of a fork.

haf commented 6 years ago

Done!

ghost commented 6 years ago

Thot is also a English slang word for "That Hoe Over There ". So no offense @MangelMaxime, but it might not be the most suitable word for marketing a library to a wider audience. After all, how should we non-native English speakers supposed to know all of that stuff in advance. 😄

MangelMaxime commented 6 years ago

@kfrie You are not the first to mention it to me, but because I tought the name of the god was written Thot two, I had no problem with that.

But in fact the god name is Thoth, so I will rename my library..

Just to mention too, that I started the work on this issue but it will take much more time to do than I expected. I don't have the time right now to do it. I prefer to be transparent :)