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

Published 20 hours ago • verified publisher icon jamesmacaulay

Upgrade to elm http2

Open torrefatto opened this issue 4 years ago • 2 comments

Hi! As we discussed earlier, here is a PR to upgrade to elm/http >= 2.0.0 and to address some of the concerns expressed in #42

In particular:

  • as in #42, the various send* functions now emit an Cmd msg
  • we expose some building block functions to allow the user to build it's graphql request via Http.request method from elm/http (for example graphQLBody and graphQLexpect)
  • we created a new Result type, that is a sort of three-state type, that carries the success or failure state without the need to double examine the result to get if the error is http-related or graphql-related
  • we tackled the issue of the error decoding, i.e. provide the graphql errors also if the decoding of the response succeeds (defaulting to an empty error list if everything goes smoothly)

What is missing is to update the documentation (indeed, elm make --doc=out.json fails). Let us know what do you think of the changes we've done so far and we will gladly update also the docs :blush:

torrefatto avatar Feb 02 '21 11:02 torrefatto

Thanks for this! I haven't had time to do a real review yet but from what I've seen it looks very good. I will try to complete the review this week.

Since this PR requires the other Travis PR for CI to work, perhaps it would be best to include the Travis config change in this PR? Otherwise I could merge the Travis PR into master and then you could rebase this one onto new master, is that what you had in mind?

jamesmacaulay avatar Feb 03 '21 15:02 jamesmacaulay

Thanks for this! I haven't had time to do a real review yet but from what I've seen it looks very good. I will try to complete the review this week.

Great!

Since this PR requires the other Travis PR for CI to work, perhaps it would be best to include the Travis config change in this PR? Otherwise I could merge the Travis PR into master and then you could rebase this one onto new master, is that what you had in mind?

Sure, I was trying to do it in #44 with no success :sweat_smile:. When the only step failing will be the elm-format one, I will merge it here (sorry for opening #44, but I need access to travis).

torrefatto avatar Feb 03 '21 16:02 torrefatto