elm-graphql icon indicating copy to clipboard operation
elm-graphql copied to clipboard

Port to elm/http 2.0.0

Open vodik opened this issue 6 years ago • 12 comments

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.

vodik avatar Dec 01 '18 23:12 vodik

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.

vodik avatar Dec 02 '18 01:12 vodik

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.

jamesmacaulay avatar Dec 03 '18 22:12 jamesmacaulay

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.

klaftertief avatar Dec 04 '18 14:12 klaftertief

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

vodik avatar Dec 04 '18 15:12 vodik

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.

klaftertief avatar Dec 04 '18 16:12 klaftertief

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

vodik avatar Dec 04 '18 17:12 vodik

@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.

jamesmacaulay avatar Dec 04 '18 18:12 jamesmacaulay

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

vodik avatar Dec 07 '18 20:12 vodik

Hello, any update on this? Thanks

Slumber86 avatar Jul 02 '19 08:07 Slumber86

@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?

sentience avatar Apr 16 '20 05:04 sentience

@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.

klaftertief avatar Apr 16 '20 06:04 klaftertief

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

emilianobovetti avatar Jan 13 '21 15:01 emilianobovetti