Skip to content

[5.x] Make sure combining queries don't read the same stream multiple times#1268

Merged
royduin merged 1 commit into
masterfrom
Jade-GG-patch-3
May 1, 2026
Merged

[5.x] Make sure combining queries don't read the same stream multiple times#1268
royduin merged 1 commit into
masterfrom
Jade-GG-patch-3

Conversation

@Jade-GG
Copy link
Copy Markdown
Collaborator

@Jade-GG Jade-GG commented Apr 23, 2026

ref: GT-2543

In our Sentry logging we noticed that sometimes, graphql queries would have their error body already seemingly read. This was puzzling at first, because there's no obvious way to have this happen.

However, in the submitPartials code, it turns out that when combiningGraphQL gets used, it will send the same error object to all of the graphql components that got combined. While I wasn't able to verify this was actually happening, the code seemed pretty clear cut and dry to me in this instance.

Because I didn't want to change anything about how the fetch function sends a responseClone instead of just the actual response data (I'm sure there's a reason, but I didn't dig for it), I instead reworked this part to work to work with concurrency by using a shared responseData object in the error object. Note that the naive way to do this would run into race conditions, so instead I store the awaitable part once, and then await it after.

Copy link
Copy Markdown
Member

@indykoning indykoning left a comment

Choose a reason for hiding this comment

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

...about how the fetch function sends a responseClone instead of just the actual response data (I'm sure there's a reason, but I didn't dig for it)

That's so the GraphqlError can extend FetchError, so if you catch a FetchError GraphqlErrors can be caught as well

throw error
}

const errorResponse = await error.response.json()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are there any other places this change should be used?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The only other possible place would be AddToCart.vue. However, this doesn't use combiningGraphQL so it's not relevant at this point.

I do notice that Graphql.vue uses combiningGraphQL, but it doesn't actually call .json() on the response when there's an error which is interesting 👀

@royduin royduin merged commit 4f1ea04 into master May 1, 2026
15 checks passed
@royduin royduin deleted the Jade-GG-patch-3 branch May 1, 2026 07:46
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.

3 participants