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

Port to elm/http 2.0.0 #42

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Port to elm/http 2.0.0 #42

wants to merge 2 commits into from

Conversation

vodik
Copy link

@vodik vodik commented Dec 1, 2018

So, I don't expect these changes to get accepted anywhere near as is, but might as kick off some discussion around how to port to the newer http package.

I personally need the new file upload support included in the newer package, and with these patches everything seems to work for me.

This code is very rough and kludgy - I was focused on getting things working first and foremost. I'm planning on continuing working on this patch and got some ideas of how things might need to be changed. So, a couple of notes:

  • The Http.request method now returns a Cmd directly, with the Expect type creating the result directly. Tasks are still supported through Http.task now, but requires working with a Resolver type. I wonder if we should copy this API (e.g. something like sendQuery and queryTask)

  • It might make sense to add and expose a expectGraphQL (for Expect) and (graphQLResolver for Resolver) and glue error handling directly in there instead of having to map it.

|> Http.toTask
|> Task.mapError (Util.convertHttpError HttpError GraphQLError)
Err err ->
Err (HttpError (Http.BadBody (Json.Decode.errorToString err)))
Copy link
Author

@vodik vodik Dec 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might make sense to also now flatten the Error type instead of nesting Http.Error.

@vodik
Copy link
Author

vodik commented Dec 2, 2018

I'm more than willing to see through the work to get this landed, but I'd like to get some feedback before I keep diving in. Thanks for your time.

@jamesmacaulay
Copy link
Owner

Thanks for this! I haven't looked closely at elm/http-2.0.0 yet, but if the update requires a major version bump then I think it might be good to bundle up bigger changes to error handling as well. I will try to take a closer look at this soon and get back to you with ideas for how to move forward.

@klaftertief
Copy link

I started working on elm/http@2 support as well, with the same motivation to support trackable file uploads. See https://github.com/klaftertief/elm-graphql/tree/elm-http-2

In elm/http there are now separate APIs for "normal" and "risky" request (withCredentials). I added a risky field to the RequestOptions for this purpose. For file uploads there is the question if this should be supported directly in this package or if this should be implemented in user-land. I added an API where you can add additional body parts (for POST requests), the former JSON body is then converted into a string part with a user definable name.

@vodik
Copy link
Author

vodik commented Dec 4, 2018

Yeah, I saw that as well. I like that change personally.

What I was exploring was, instead of trying to wrap the various variations of Http.request (at there's now Http.request, Http.riskyRequest, Http.task and Http.riskyTask) - on top of the existing GraphQL senders (e.g. sendQuery, customSendQuery and customSendQueryRaw) - was to try and provide the pieces to make it really easy to write custom Http code:

Http.post
    { url = "/graphql"
    , body = graphQLBody request
    , expect = expectGraphQL request GotQueryResponse
    }

That just needs one more helper:

graphQLBody request =
    postBody (Builder.requestBody request) (Builder.jsonVariableValues request)

where request is Builder.Request operationType result. The new Http API is simple enough it might be easier to drop the customable calls. And getting a raw response now as simple as switching to expect = Http.expectString

@klaftertief
Copy link

Sounds like a good plan. )I do not really like my changes.)
A graphQLResolver would also be needed for a Task based API, but the implementation is already contained in expectGraphQL.

@vodik
Copy link
Author

vodik commented Dec 4, 2018

Yeah, I just wanted an endorsement that @jamesmacaulay likes the approach before spending much time on it.

@jamesmacaulay
Copy link
Owner

@vodik I like that approach of just giving people good building blocks to make their own requests, I considered it before but it's looking like it makes more and more sense now. Error handling is the biggest question mark in my mind, but I think it should be done with the same philosophy: just make good building blocks available instead of trying to make a one-size-fits-all wrapper.

@vodik
Copy link
Author

vodik commented Dec 7, 2018

Awesome. I'll try and get this done over the weekend then.

@Slumber86
Copy link

Hello, any update on this?
Thanks

@sentience
Copy link

@vodik @klaftertief I take it, for whatever reason, you have not been able to progress in this work? Have you gone to a different GraphQL client library, or do you have any other recommendations for folks needing to upgrade to elm/http 2.0?

@klaftertief
Copy link

@sentience We had a patched version for some time in our project and switched to dillonkearns/elm-graphql later. Mostly because of the type-safety.

@emilianobovetti
Copy link

Hello 👋 is there any plan to work on this port? I'd be glad to help!

@torrefatto torrefatto mentioned this pull request Feb 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants