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

Fix for ServerError obscured by ServerParseError #9997

Merged

Conversation

dillontiner
Copy link
Contributor

@dillontiner dillontiner commented Aug 8, 2022

Fixes #8945 where not handling the response status code before parsing the response body text causes the apollo client to send a ServerParseError when it should send a ServerError.

The changes here are taken from #8974 which has been inactive for months

@apollo-cla
Copy link

@dillontiner: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@dillontiner dillontiner mentioned this pull request Aug 8, 2022
@jpvajda jpvajda self-assigned this Sep 1, 2022
@jpvajda
Copy link
Contributor

jpvajda commented Sep 1, 2022

@dillontiner Thanks for follow up on this and our apologies for the lingering PR review, we'll check this out soon. 🙏

Copy link
Contributor

@jpvajda jpvajda left a comment

Choose a reason for hiding this comment

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

This LGTM but I'm going to ask another person on the team to give this a quick check. cc @hwillson @alessbell @MrDoomBringer @benjamn

Once merged we can close out:

@jpvajda
Copy link
Contributor

jpvajda commented Sep 1, 2022

This seems to be failing the NPM Bundle Size check

@jpvajda jpvajda removed their assignment Sep 6, 2022
@MrDoomBringer MrDoomBringer self-assigned this Sep 12, 2022
@MrDoomBringer
Copy link
Contributor

LGTM. I went ahead and added some extra tests - we're now testing unparsable JSON with both 200 and 400 server response codes. Other than that, I'm glad to get this merged! Thanks a ton @TakahiroHimi for the original PR and @dillontiner for keeping the fix alive.

All the best,
Emmanuel, Intern :-)

@MrDoomBringer MrDoomBringer merged commit d16b3c4 into apollographql:main Sep 22, 2022
@dillontiner dillontiner deleted the fix-server-parse-error-order branch February 10, 2023 19:38
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parse http response
4 participants