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 type signature of ServerError #10810

Merged
merged 7 commits into from
May 3, 2023
Merged

Conversation

dleavitt
Copy link
Contributor

ServerError#result will be a string when the response has a >= 300 status code and isn't json.

It looks like maybe there was a breaking change in parseAndCheckHttpResponse.ts when adding defer support in 3.7.

  • Previously, a non-json response would always result in a ServerParseError. So ServerError's result would always be JSON.
  • Now, the status code check happens first so you get a ServerError (with a string result) instead.

Maybe this is a bug? The old behavior seems better.

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)

`result` can be a string
@netlify
Copy link

netlify bot commented Apr 27, 2023

‼️ Deploy request for apollo-client-docs rejected.

Name Link
🔨 Latest commit bd1e0fd

@apollo-cla
Copy link

@dleavitt: 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/

@changeset-bot
Copy link

changeset-bot bot commented Apr 27, 2023

🦋 Changeset detected

Latest commit: f17b34b

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@alessbell alessbell added the 🏓 awaiting-team-response requires input from the apollo team label Apr 28, 2023
@alessbell alessbell self-assigned this May 2, 2023
@alessbell
Copy link
Contributor

Hi @dleavitt 👋 Thanks for opening this PR!

You're correct that the behavior slightly changed here in 3.7: previously, if a request returned a response with >= 300 status code and string body, ServerError.message would have been a JSON parse error, e.g. "Unexpected token 'E', \"Error! Foo bar\" is not valid JSON" and ServerError.result was undefined. In 3.7 and later, ServerError.message is Response not successful: Received status code 302 and ServerError.result is the string from the response.

I've added a test case and small related TS fix: expanding ServerError.result to include string caused TS errors in the persisted query link where network errors are being cast to GraphQLErrors for a very specific reason (see: #9410); reworking the way that type is narrowed allows us to accurately reflect reality in the ServerError type.

@alessbell alessbell requested review from jerelmiller and phryneas May 2, 2023 18:53
@alessbell alessbell removed the 🏓 awaiting-team-response requires input from the apollo team label May 2, 2023
@alessbell alessbell requested a review from benjamn May 2, 2023 20:00
Copy link
Member

@jerelmiller jerelmiller left a comment

Choose a reason for hiding this comment

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

This looks great to me! Thanks for getting this fixed! 🎉

@alessbell alessbell merged commit a625277 into apollographql:main May 3, 2023
This was referenced May 3, 2023
@dleavitt
Copy link
Contributor Author

dleavitt commented May 3, 2023

@alessbell good catch, thanks so much!

@dleavitt dleavitt deleted the patch-2 branch May 3, 2023 22:49
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants