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(core): move multipart/mixed to end of accept headers #3039

Merged
merged 4 commits into from
Mar 14, 2023

Conversation

JoviDeCroock
Copy link
Collaborator

@JoviDeCroock JoviDeCroock commented Mar 14, 2023

Summary

Follow up to #3007

In React-Native-Fetch streaming isn't supported meaning that the inclusion of multipart/mixed can lead to errors. We move it to the end of the Accept headers to avoid running into this bug. This was reported on Discord.

Relates to facebook/react-native#27741

@JoviDeCroock JoviDeCroock requested a review from kitten March 14, 2023 12:38
@JoviDeCroock JoviDeCroock changed the title fix(core): only add multipart/mixed when streaming directives are found fix(core): move multipart/mixed to end of accept headers Mar 14, 2023
@kitten
Copy link
Member

kitten commented Mar 14, 2023

Just noting based on discussions off GitHub; This isn't an issue with React Native per se, however, GraphQL Yoga seems to match the first transport method in Accept that can be used to fulfil a request, rather than defaulting to the standard JSON response and using a multipart one as needed.

This means that Yoga seems to trigger a multipart response, which is unsupported on React Native, even when this isn't stricly necessary, failing requests on RN entirely.

.changeset/early-crabs-draw.md Outdated Show resolved Hide resolved
Co-authored-by: Phil Pluckthun <phil@kitten.sh>
@JoviDeCroock JoviDeCroock merged commit 77f6f3a into main Mar 14, 2023
@JoviDeCroock JoviDeCroock deleted the selectively-add-multipart-mixed branch March 14, 2023 13:08
This was referenced Mar 14, 2023
@github-actions github-actions bot mentioned this pull request Mar 27, 2023
@ericbf
Copy link

ericbf commented Apr 10, 2023

Is this part of a release?

@JoviDeCroock
Copy link
Collaborator Author

@ericbf above your comment there is a linked merged PR that indicates the version this is part of... #3108

@ericbf
Copy link

ericbf commented Apr 10, 2023

Ok, so that’s what that means. I was wondering if that corresponded to a release (like on npm). Thank you!

@ericbf
Copy link

ericbf commented Apr 11, 2023

Because we’re on "urql": "^4.0.0" and we’re seeing an accept header of multipart/mixed, application/graphql-response+json, application/graphql+json, application/json. Should I open a bug report?

@ericbf
Copy link

ericbf commented Apr 11, 2023

After scouring the docs some more, seems like it due to still having @urql/exchange-multipart-fetch in there from before migrating (painfully) to version 4. A migration guide would have been helpful... Is there one?

image

It’s also worth mentioning that the package was actually @urql/exchange-multipart-fetch, not @urql/multipart-fetch-exchange.

@kitten
Copy link
Member

kitten commented Apr 11, 2023

@ericbf Yes, there is one: #3114
If you have amy questions please instead however open a discussion thread here on GitHub or on Discord. And yes, the package name is a typo. Re. still seeing old headers, you likely haven't deduplicated @urql/core, because even with old exchanges, which are still compatible, you should try to always keep only one version in your bundle/app. The migration guide has instructions on deduplicating however

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