v6: fix transport error metadata, subscription teardown, and the spec accept header#4429
Draft
trevor-scheer wants to merge 6 commits into
Draft
v6: fix transport error metadata, subscription teardown, and the spec accept header#4429trevor-scheer wants to merge 6 commits into
trevor-scheer wants to merge 6 commits into
Conversation
`create-fetcher/lib.ts` now reads the response body as text and falls back to a synthetic error payload instead of throwing when it isn't JSON, so a non-JSON error response (an HTML proxy page, a plain-text 401, an empty 204) still carries `status`/`statusText`/`headers` instead of rejecting into a generic error. `multipartHttpTransport`'s accept header now includes `application/graphql-response+json` alongside `application/json, multipart/mixed`, matching the simple path. Incremental delivery is on by default, so this was the accept header most requests were actually sending. `ok` is now `response.ok && !hasErrors` instead of only consulting the GraphQL body, so a 401 or 500 is never `ok: true` just because the body happens to parse as JSON with no top-level `errors`. Add `extensions` and `signal` to `TransportRequest`/`FetcherParams`, threaded through `buildGetUrl`, the POST/QUERY body, and the underlying `fetch` call. `createTransport.ts` also now parses the query once per `send()` instead of twice (once to check for a subscription, once to check for a mutation), and the unused `ResolverTrace` reservation is dropped from `TransportResponse.timing` since it was never populated.
`execution.ts`'s subscription and fetcher async-iterable paths called `result[Symbol.asyncIterator]()` a second time inside `unsubscribe` to get something to call `.return()` on. `transport-hooks.ts#wrap` mints a fresh iterator (with its own independent state) on every `[Symbol.asyncIterator]()` call, so that second call built a throwaway iterator, called `.return()` on it, and left the real iterator driving the `for await` loop completely untouched. Stop did nothing: the underlying `subscriptionClient` was never disposed and the socket/SSE connection kept running. Both paths now capture the iterator once and drive/dispose that same object. Also thread an `AbortSignal` through the `transport` query/mutation path: `run()` creates an `AbortController` per request, passes its signal into `transport.send()`, and `stop()` now aborts it alongside unsubscribing. A `stop()`-triggered abort is treated separately from a genuine request failure, so it clears fetching state without dispatching an error into the response pane.
`TransportHookRegistry` exposed `onBeforeSend` (transform) and `onResponse` (observe-only), but nothing fired when `send()` rejected, so a plugin had no way to observe a failed request. Add `onError`, called with the error and the outgoing request whenever `wrap()`'s iterator throws, whether from a rejected `transport.send()`, a rejected iterable, or a throwing `onBeforeSend` callback. It's observe-only like `onResponse`: the error still propagates to the caller after every registered callback runs. Exposed on the plugin context alongside the other two hooks.
…Client typo Add the `extensions` and `signal` fields to the `send()` example, and add `extensions` to the `SubscriptionClient` contract snippet. Also fix a stale reference to `legacyClient`, which was renamed to `legacyWsClient`.
Fold the error-body handling, accept header, ok semantics, extensions, signal, and onError additions into the existing transport-api entry rather than adding a new changeset — the Transport API is still unreleased on this branch, so these are part of the same 6.0.0 changelog entry.
|
| Name | Type |
|---|---|
| @graphiql/toolkit | Minor |
| @graphiql/react | Minor |
| graphiql | Minor |
| @graphiql/plugin-history | Major |
| @graphiql/plugin-code-exporter | Major |
| @graphiql/plugin-collections | Major |
| @graphiql/plugin-doc-explorer | Major |
| @graphiql/plugin-query-builder | Major |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
A 502 with a plain-text `Bad Gateway` body used to reach the user as "Unexpected token 'B', ... is not valid JSON" — the JSON parser's complaint, not the server's. It now surfaces as `Bad Gateway`. Drops the browser-dependence caveat with it: the message is the response body now, not whatever the local JSON parser happened to say.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three behavioral fixes to the transport layer, plus two additive request fields, before
Transportfreezes at6.0.0.create-fetcher/lib.tscalledresponse.json()unconditionally. A non-JSON error body (an HTML page from a proxy, a plain-text 401, an empty 204) made.json()throw, and the rejection collapsed into a generic error string exactly when the caller most neededstatus/statusText/headers. The body is now read as text first; if it parses as JSON it's used as-is, otherwise the raw text becomes the error message, but either way a realTransportResponsecarrying the actual HTTP status comes back instead of a rejection.transport-hooks.ts#wrap) hands back an object whose[Symbol.asyncIterator]()mints a fresh iterator on every call.execution.ts's unsubscribe logic called that method a second time to get something to.return(), which built a throwaway iterator and closed it while the real one, still driving thefor awaitloop, kept running untouched. Clicking Stop looked like it worked but the underlying socket or SSE connection just kept going. Both the transport and fetcher paths now capture the iterator once and drive/dispose that same object.multipartHttpTransport, whose accept header wasapplication/json, multipart/mixedwith noapplication/graphql-response+json. Out of the box this asked spec-compliant servers to fall back to legacy response semantics. Both paths now send the same accept header.extensionsandsignaltoTransportRequest.extensionsrides along GraphQL-over-HTTP requests for things like persisted queries, and was already claimed (incorrectly) by a doc comment that never encoded it.signalis a plainAbortSignal;run()now creates anAbortControllerper query/mutation andstop()aborts it, so Stop actually cancels an in-flight request instead of doing nothing. (Subscription Stop is the leak fix above, not the abort signal — sockets need real teardown, not just an aborted fetch.)A few judgment calls, made rather than left as an inconsistent matrix:
okis nowresponse.ok && !hasGraphQLErrorsinstead of only looking at the GraphQL body. A 401 or 500 was previouslyok: trueif the body happened to parse as JSON with noerrorsarray, which collides with whatResponse.okmeans everywhere else. Subscriptions have no HTTP status to consult, so theirokstays purely GraphQL-error-based.onErrorhook to the plugin transport context, alongside the existingonBeforeSend/onResponse. Without it a plugin had no way to observe a request that failed outright (network error, thrownonBeforeSendhook); for a frozen plugin API that asymmetry seemed like the likeliest regret to leave unaddressed.ResolverTracetype andtiming.resolverTracesfield. Adding fields later is non-breaking, so reserving them now bought nothing and just shipped a field that always readsundefined.wrap()'s hand-rolled async iterator alone rather than rewriting it as anasync function*. The fix above (capture the iterator once, don't call[Symbol.asyncIterator]()twice) is the actual bug; a generator rewrite is a real simplification but a separate, riskier change.This PR also updates
create-transport/README.md: documents the newextensions/signalfields and fixes a stalelegacyClientreference (should belegacyWsClient).Test plan
/graphqlin devtools and force an HTML/plain-text error response) and confirm the response pane shows a real500status badge, not a generic error message.Acceptincludesapplication/graphql-response+json.extensionsset on aTransportand confirm it shows up in the request (query string for GET, JSON body for POST/QUERY).yarn workspace @graphiql/toolkit test && yarn workspace @graphiql/react testpasses.Refs: #4219