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

Better serialization on graphql queries and refactoring #596

Merged
merged 7 commits into from
Jun 20, 2022

Conversation

hariroshan
Copy link
Contributor

Regarding Feedback
I've made the changes to serialisation and decoding of GraphQL queries. Review and Merge accordingly 😄

@dillonkearns
Copy link
Owner

Hi @hariroshan, thank you for the PR! There are some failing tests (see the GitHub action). Could you fix those, and also add a few unit tests to make sure we have good coverage around cases where hashes are used?

@dillonkearns
Copy link
Owner

Hi @hariroshan, thanks for fixing the unit tests in DocumentTests.elm.

Could you also add some additional unit test cases in that file that show the cases where hashes are still used to make sure those are working properly (and continue to work properly with any future changes)? Besides that, this is looking good to merge 👍

@hariroshan
Copy link
Contributor Author

Can you check for any changes or additions?

@dillonkearns dillonkearns merged commit e351b0d into dillonkearns:master Jun 20, 2022
@dillonkearns
Copy link
Owner

This is now live with Elm package version 5.0.10. Thank you so much for your work on this @hariroshan, I really appreciate it! 🙏

https://github.com/dillonkearns/elm-graphql/blob/master/CHANGELOG-ELM-PACKAGE.md#5010---2022-06-20

@hariroshan
Copy link
Contributor Author

hariroshan commented Jun 20, 2022

@dillonkearns Pleasure is mine. Thank you for maintaining this awesome package. Also do u have any idea on how we can implement persistent queries?. I'd like to contribute if you know how we can achieve this

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.

2 participants