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

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

Open
MangelMaxime opened this issue Feb 27, 2018 · 15 comments
Open

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

MangelMaxime opened this issue Feb 27, 2018 · 15 comments

Comments

@MangelMaxime
Copy link
Collaborator

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.

  • Hopac + HttpClient for NetCore runtime
  • XMLHttpRequest for Fable runtime

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

@haf
Copy link
Owner

haf commented Feb 27, 2018

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
Copy link
Collaborator Author

MangelMaxime commented Feb 27, 2018

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.

  • We will need to publish two differents packages in order to not include Hopac and System.Diagnostics.FileVersionInfo dependencies on Fable apps. And same for the DotNet version

  • Because we will need to publish two packages, we will need 2 fsproj

  • Should we split HttpFs.fs file in two separate files like:

    • Shared.fs: Contains shared types (between Fable and NetCore version) like HttpMethod
    • HttpFs.fs: Contains DotNet core part like the runnner and code used to transform a Request into an HttpRequestMessage

    I know that splitting the file would prevent from include Http.fs via paket-files.

  • We need to run test over Fable too, so we need to setup all the config needed too

    • npm dependencies
    • Fable test can now be run over Mocha (for the JS side) and Expecto for the dotnet side. We can write the same code and just choose which version to run with some helpers

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.

  • Both Fable env and dotnet env is setup up (sti
  • The repo is already setup to run test over Mocha and Expecto
  • The docs site is already created

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:

  • We can't set custom cookies from the browser, we only got the option:
      /// If set to true, then the cookies are send with the request
      WithCredentials : bool

This is a limitation compare to Http.fs

  • We describe the expecting result:
      /// 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

  • We have a cacheBuster option:
      CacheBuster : string option
      /// Query params to add to the request

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

  • Thot.Json will never generate an Exception, the result of a request is Result<'Out,HttpError>
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
Copy link
Owner

haf commented Feb 27, 2018

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
Copy link
Collaborator Author

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
Copy link
Owner

haf commented Feb 27, 2018

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
Copy link
Collaborator Author

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
Copy link
Owner

haf commented Feb 27, 2018

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

@haf
Copy link
Owner

haf commented Feb 27, 2018

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
Copy link
Owner

haf commented Feb 27, 2018

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
Copy link
Owner

haf commented Feb 27, 2018

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

@MangelMaxime
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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
Copy link
Owner

haf commented Feb 28, 2018

Done!

@ghost
Copy link

ghost commented Apr 7, 2018

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
Copy link
Collaborator Author

@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 :)

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