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

[graphql-client] Improves error handling for missing Response.body in multipart requests #1998

Merged
merged 1 commit into from
Jan 22, 2025

Conversation

ecbrodie
Copy link
Contributor

@ecbrodie ecbrodie commented Jan 21, 2025

WHY are these changes introduced?

This fix was motived by development of an internal library that serves as a consumer of the graphql-client package.

We detected instances of errors in multipart GraphQL requests where, for some reason, Response.body was undefined despite a 200 OK response. This seems to occur mostly (if not always) on frontends that use a custom window.fetch implementation.

The symptom of this error would be the following message: Cannot read properties of undefined (reading 'Symbol(Symbol.asyncIterator)').

I deduced this error to be caused by a check in createMultipartResponseAsyncInterator() where there is an assumption, via response.body![Symbol.asyncIterator], that response.body is always defined, especially for responses with a truthy response.ok value. Clearly this is not a safe assumption to make, especially in web environments where the fetch implementation acts in unexpected ways.

WHAT is this pull request doing?

By using optional chaining instead of the non-null assertion operator, we avoid an unexpected behaviour from attempting to read a property from an undefined response.body and instead fall back to throwing the existing error message, which I believe is indeed the more expected result of this situation: API multipart response did not return an iterable body.

Type of change

  • Patch: Bug (non-breaking change which fixes an issue)
  • Minor: New feature (non-breaking change which adds functionality)
  • Major: Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

  • I have used pnpm changeset to create a draft changelog entry (do NOT update the CHANGELOG.md files manually)
  • I have added/updated tests for this change
  • I have documented new APIs/updated the documentation for modified APIs (for public APIs)

@ecbrodie ecbrodie requested a review from a team as a code owner January 21, 2025 23:06
@ecbrodie ecbrodie requested a review from Copilot January 21, 2025 23:06

Choose a reason for hiding this comment

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

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Copy link
Contributor

@lizkenyon lizkenyon left a comment

Choose a reason for hiding this comment

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

Thank you! ⭐

'@shopify/graphql-client': patch
---

Better error handling for missing Response.body in multipart requests
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's explicitly say that now API multipart response did not return an iterable body is thrown in this case.

@ecbrodie
Copy link
Contributor Author

@lizkenyon are you able to also confirm here that the failing Changelog Check does not impact this PR's ability to merge?

After I apply your review suggestion, would you like me to trigger the merge or is that more for you to do?

@ecbrodie ecbrodie force-pushed the graphql-client-undefined-body-detection branch from 4acad02 to d3531c5 Compare January 22, 2025 20:22
@lizkenyon lizkenyon enabled auto-merge January 22, 2025 20:24
@lizkenyon lizkenyon merged commit e02a27c into main Jan 22, 2025
6 checks passed
@lizkenyon lizkenyon deleted the graphql-client-undefined-body-detection branch January 22, 2025 20:33
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