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

Deprecate decoding using atoms #69

Merged
merged 13 commits into from
Sep 12, 2022
Merged

Conversation

c0m3tx
Copy link
Contributor

@c0m3tx c0m3tx commented Sep 1, 2022

Updated Client.call function, which now also accepts a decoder (just before options). Decoder is just a function with arity 1 which accepts a body and returns a tuple {:ok, decoded_body} or {:error, any}. Since the arity of the call function has changed, I also created a (deprecated) function with the old arity which fallbacks to atoms keeping the current behavior intact.

Updated the BridgeEx.Graphql macro, which now accepts the decoder too, and prints a warning when it's not provided (since it falls back to atom decoding).

Also provided three basic decoders in the Utils module (to decode keys in strings, atoms and existing_atoms) which simply mirror Jason behavior. This approach allows lib users to define their own decode functions which may be useful for some applications.

@c0m3tx c0m3tx requested a review from Fs00 September 1, 2022 14:35
@Fs00 Fs00 requested review from cpiemontese and fabuloso September 1, 2022 14:39
@cpiemontese cpiemontese requested a review from a team September 2, 2022 07:32
lib/graphql/utils.ex Outdated Show resolved Hide resolved
@c0m3tx c0m3tx requested a review from cpiemontese September 2, 2022 10:56
@cpiemontese cpiemontese requested a review from a team September 2, 2022 10:58
cpiemontese
cpiemontese previously approved these changes Sep 2, 2022
@cpiemontese cpiemontese requested review from a team and cottinisimone September 2, 2022 13:51
@cpiemontese
Copy link
Contributor

Nice job 😄 I don't know if someone has anything else to say

Copy link
Contributor

@Fs00 Fs00 left a comment

Choose a reason for hiding this comment

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

Overall the approach is good, I'd change a couple of things:

  • I would rename the decoder param to decode_keys everywhere, to be more consistent with the naming of encode_variables and format_variables
  • I would personally restrict the decode_keys option to just :atoms and :strings, because:
    • I don't think the behavior of the existing_atoms is something desirable (throwing an error doesn't look very robust)
    • I feel that providing a custom decoder function is a bit out of scope for the goal in the Roadmap and I don't see any use-cases for that

lib/graphql/client.ex Outdated Show resolved Hide resolved
lib/graphql/client.ex Show resolved Hide resolved
lib/graphql/client.ex Outdated Show resolved Hide resolved
@c0m3tx
Copy link
Contributor Author

c0m3tx commented Sep 6, 2022

:existing_atoms may be useful when you have to strictly check the validity of what you get in response. Raising an error is not always something undesired, you can optionally catch the exception and behave accordingly. It's just "another option" which doesn't hurt IMHO.

As for the custom decoder, I think it's worthwhile since it would allow decoding non-json endpoints like, for instance, gRPC (or XML 😱). Having custom decoders is almost "zero effort" on this lib side, and allows more flexibility on the user side.

@Fs00
Copy link
Contributor

Fs00 commented Sep 6, 2022

:existing_atoms may be useful when you have to strictly check the validity of what you get in response.

Ok, you convinced me 😄

it would allow decoding non-json endpoints like, for instance, gRPC (or XML 😱)

Non-JSON endpoints are not supported by the library (since it's fully committed to GraphQL) and won't be for the foreseeable future.
In general, I prefer to keep the API surface as small and focused as possible for simplicity and to avoid potential pitfalls. In this particular case I see that the behavior of a custom decoder may overlap with the format_response option, which already massages the response keys.
I think it's better to avoid dealing with the interaction (or potential conflicts) between these two options 🙂

@c0m3tx
Copy link
Contributor Author

c0m3tx commented Sep 6, 2022

may overlap with the format_response option, which already massages the response keys. I think it's better to avoid dealing with the interaction (or potential conflicts) between these two options slightly_smiling_face

Ah yes, I didn't consider it at all! Ok you convinced me this time 😄

@c0m3tx c0m3tx requested a review from Fs00 September 6, 2022 13:53
Copy link
Contributor

@Fs00 Fs00 left a comment

Choose a reason for hiding this comment

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

I've proposed a few nitpicks/improvements to the documentation, the rest is already looking good

README.md Outdated Show resolved Hide resolved
lib/graphql/client.ex Outdated Show resolved Hide resolved
lib/graphql.ex Outdated Show resolved Hide resolved
lib/graphql.ex Outdated Show resolved Hide resolved
lib/graphql/client.ex Outdated Show resolved Hide resolved
@c0m3tx c0m3tx requested a review from Fs00 September 7, 2022 07:21
Copy link
Contributor

@Fs00 Fs00 left a comment

Choose a reason for hiding this comment

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

I've pushed a small fix for an unrelated typo that screwed up the documentation on HexDocs.
LGTM!

Copy link
Contributor

@cpiemontese cpiemontese left a comment

Choose a reason for hiding this comment

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

Nice 🎉

@cpiemontese cpiemontese merged commit 9b9ed5c into master Sep 12, 2022
@cpiemontese cpiemontese deleted the deprecate-decoding-using-atoms branch September 12, 2022 09:48
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.

4 participants