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

Upgrade to elm http2 #43

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

Conversation

torrefatto
Copy link
Contributor

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 Port to elm/http 2.0.0 #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 😊

@torrefatto torrefatto mentioned this pull request Feb 2, 2021
@jamesmacaulay
Copy link
Owner

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?

@torrefatto
Copy link
Contributor Author

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 😅. 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).

- Fix elm/http-2-related issues
- Create and expose utility functions to be used by library user
- Overhaul the error management
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.

3 participants