search

dillonkearns / elm-graphql

Autogenerate type-safe GraphQL queries in Elm.
https://package.elm-lang.org/packages/dillonkearns/elm-graphql/latest
BSD 3-Clause "New" or "Revised" License
778 stars 107 forks source link

Add operationName to mutation request body #566

Closed galdiuz closed 2 years ago

galdiuz commented 2 years ago

Closes #565.

Here is a PR to add operationName to Mutation requests. Please let me know if there's anything you want me to change.

galdiuz commented 2 years ago

I should note that I added this in the most simple way possible, as I was unsure if it would be more appropriate to use QueryHelper.build. If you think that is a better way I could change to use that instead.

dillonkearns commented 2 years ago

Thank you @galdiuz! Could you add some test cases to exercise the new code path?

I believe the test file to add the cases to would be https://github.com/dillonkearns/elm-graphql/blob/63de2e2868bbdff43eb9e4d109114ff34a00e45e/tests/Http/QueryHelperTests.elm.

dillonkearns commented 2 years ago

Ah yes, having that logic in QueryHelper.build does seem like a good idea. That way the logic is co-located. The Http module should just be delegating to outside for the logic on how to construct the URL with the query in it, so yes please do make that change 🙏

galdiuz commented 2 years ago

After reading through and actually understanding QueryHelper I believe that Mutations actually don't need to use it. As I understand QueryHelper solves the issue of selecting between GET and POST and building the URL and body as appropriate, but since Mutations are always POST they have no need for this logic. I could add a buildMutation to QueryHelper or create a new MutationHelper, but any code within it would basically only be a copy of what's already in the Mutation branch of Http.toReadyRequest so it feels unnecessary. If you disagree and would rather have it separated I'd be up for writing that code and any associated tests though, so please let me know what you think.

galdiuz commented 2 years ago

Bump @dillonkearns. You're usually quick to respond so I figured you might've missed my response (and I apologize if that isn't the case).

dillonkearns commented 2 years ago

Yeah, looks like the concerns are mixed between those two modules a bit already, so the addition in the Http module for now seems good 👍 Thanks for the PR!

galdiuz commented 2 years ago

@dillonkearns Could you please add a new tag so that this is available in Elm Packages?

dillonkearns commented 2 years ago

The fix is live in version 5.0.7 now 👍 Thanks again for the PR!

https://github.com/dillonkearns/elm-graphql/blob/master/CHANGELOG-ELM-PACKAGE.md#507---2021-10-26