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

Improve ResponseCodeError API #2426

Closed
AnthonyMDev opened this issue Aug 4, 2022 · 6 comments · Fixed by #3123
Closed

Improve ResponseCodeError API #2426

AnthonyMDev opened this issue Aug 4, 2022 · 6 comments · Fixed by #3123
Labels
good first issue Issues that are suitable for first-time contributors. help wanted Issues the Apollo team would love help from the community about v1.x

Comments

@AnthonyMDev
Copy link
Contributor

This error is really difficult to parse information from. See #2419 for more info.

@AnthonyMDev AnthonyMDev added help wanted Issues the Apollo team would love help from the community about good first issue Issues that are suitable for first-time contributors. v1.x labels Aug 4, 2022
@dfperry5
Copy link
Contributor

@AnthonyMDev Any chance this is still an open issue? :) I wouldn't mind taking it on. This would be my first time contributing to Apollo iOS though.

@AnthonyMDev
Copy link
Contributor Author

I'd be happy to have you work on this @dfperry5! Thanks for offering to contribute.

It actually looks like the data does contain a spec compliant GraphQLError response. So we should probably be exposing that data as a GraphQLError struct.

The right way to do this would involve separating the JSON serialization interceptor from the parsing of the actual GraphQL result, but that is going to require a lot of restructuring of our network APIs that we are going to do in the 2.0 release.

But in order to improve this for now without breaking things, I think the right approach would be for us to:

  • Add a new optional computed property graphQLError: GraphQLError? to ResponseCodeError.
    • This computer property would deserialize the JSON itself and if it is able to parse then parse the error from that into a GraphQLError struct if it exists.

@dfperry5
Copy link
Contributor

Awesome! @AnthonyMDev - I will give it a try!

@dfperry5
Copy link
Contributor

dfperry5 commented Jul 8, 2023

@AnthonyMDev - could you take a peak at this? https://github.com/apollographql/apollo-ios/compare/main...dfperry5:apollo-ios:Issue-2426-Improve-ResponseCodeError-API?expand=1

It's my first pass, I'm not overly thrilled with it :P Not sure if there is something more obvious I am missing.

@AnthonyMDev
Copy link
Contributor Author

This looks like what we want right now. If you can just add another test that an error that doesn't have a GraphQLError returns nil from error.graphQLError and then add a line of documentation to the graphQLError property this should be ready for a PR!

Thanks so much for doing this!

@dfperry5
Copy link
Contributor

#3123 :) Thanks for guiding me! Let me know if there are any chances.

@BobaFetters BobaFetters linked a pull request Jul 20, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Issues that are suitable for first-time contributors. help wanted Issues the Apollo team would love help from the community about v1.x
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants