-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Make protocol errors available in the error link #12281
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: e54cfda The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 1 changed, 0 removed
Build ID: b04d6bfd4e0ed73cab77de3e URL: https://www.apollographql.com/docs/deploy-preview/b04d6bfd4e0ed73cab77de3e |
commit: |
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -8,6 +16,10 @@ import { ApolloLink } from "../core/index.js"; | |||
export interface ErrorResponse { | |||
graphQLErrors?: ReadonlyArray<GraphQLFormattedError>; | |||
networkError?: NetworkError; | |||
protocolErrors?: ReadonlyArray<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point, these errors become confusing without explanation or context.
What about some comments?
/**
* Errors returned in the `errors` property of the GraphQL response.
*/
graphQLErrors?: ReadonlyArray<GraphQLFormattedError>;
/**
* Errors thrown during making a network request.
* This is usually an error thrown during a `fetch` call or an error while
* parsing the response from the network.
*/
networkError?: NetworkError;
/**
* Protocol errors.
* This type of error occurs in follow-up responses of HTTP multipart
* responses, e.g. if you are using `@defer` or Subscriptions via HTTP multipart.
*/
@@ -8,6 +16,10 @@ import { ApolloLink } from "../core/index.js"; | |||
export interface ErrorResponse { | |||
graphQLErrors?: ReadonlyArray<GraphQLFormattedError>; | |||
networkError?: NetworkError; | |||
protocolErrors?: ReadonlyArray<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we call those "protocol errors" internally, but looking at when these actually occur (only on subsequent chunks of http multipart requests), could we maybe call this incrementalErrors
instead?
protocolErrors?: ReadonlyArray<{ | |
incrementalErrors?: ReadonlyArray<{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I named this protocolErrors
to match the same property used in ApolloError
where these specific types of errors are stored.
These errors aren't actually incremental errors but rather the transport-level errors sent from the router that occur when it can't communicate with the subgraph (i.e. the errors
field next to payload
, but not the errors
field inside the payload
property, which are just graphQLErrors
).
]); | ||
}); | ||
|
||
const mockLink = new ApolloLink((_operation) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes a lot of assumptions about what the HttpLink will do. I'll see if I can rewrite the test to use an actual HttpLink
I've added tests actually using an I had assumed that this would already also cover |
@@ -53,7 +53,7 @@ export interface ApolloPayloadResult< | |||
payload: SingleExecutionResult | ExecutionPatchResult | null; | |||
// Transport layer errors (as distinct from GraphQL or NetworkErrors), | |||
// these are fatal errors that will include done: true. | |||
errors?: ReadonlyArray<Error | string>; | |||
errors?: ReadonlyArray<GraphQLFormattedError | string>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed this while writing the test - this is not a native Error
object, but a JSON-parsed plain object with a message
and potentially more fields. This type seems more fitting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense. According to the protocol:
Both types of
errors
follow the GraphQL error format, but top-levelerrors
never includelocations
orpath
.
This does make me question the string
type here given that statement. I wonder if it would also make sense to add & { locations?: never; path?: never }
to the type to make it even more accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, admittedly, that string type seems kinda off.
As for the & { locations?: never; path?: never }
- we could do that, but I wouldn't sweat it too much honestly - it's already optional, so nobody should be relying on those.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally, I'd approve this PR, but I also want to suggest that we extend it to also handle @defer
incremental errors and move both of those types of errors together under an incrementalErrors
field.
Alternatively, we could keep this as "protocol errors" (but I'm not sure we use that name in any user-facing documentation) and expose @defer
errors as graphQLErrors
in a future PR.
protocolErrors?: ReadonlyArray<{ | ||
message: string; | ||
extensions?: GraphQLErrorExtensions[]; | ||
}>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could also just be ReadonlyArray<GraphQLFormattedError>
, or we adjust ApolloPayloadResult
to have a type matching this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the type defined in ApolloError
for this. I figured they should match.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe it might also make sense to just reference ApolloPayloadResult['errors']
to create a strong connection showing where this comes from?
Discussed in person. We are going to pull the work for |
Fixes #12258
Makes protocol errors from multipart subscriptions available in the error link. Access it with the
protocolErrors
property: