Separate error objects #12398
Separate error objects #12398
Conversation
🦋 Changeset detectedLatest commit: ec9351c 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 |
|
commit: |
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-restricted-types | ||
| export type GraphQLErrors = ReadonlyArray<GraphQLError>; | ||
|
|
||
| export type NetworkError = Error | ServerParseError | ServerError | null; |
There was a problem hiding this comment.
I'd be curious your thoughts on whether we should keep this type or not. Looks like its used for the onError link for the networkError property.
There was a problem hiding this comment.
I'd go for an ErrorLike since the Error interface and Error class of TS overlap, which feels wonky.
There was a problem hiding this comment.
Will leave this for now and revisit in a future PR when we touch onError to take an ErrorLike argument rather than the separate properties.
|
|
||
| const { hasOwnProperty } = Object.prototype; | ||
|
|
||
| export type ServerParseError = Error & { |
There was a problem hiding this comment.
This is now an error subclass so instanceof works 🎉
jerelmiller
force-pushed
the
jerel/consolidate-error-properties
branch
from
56937ce to
a8800ff
Compare
jerelmiller
force-pushed
the
jerel/separate-error-objects
branch
from
33068df to
3ea9357
Compare
jerelmiller
force-pushed
the
jerel/consolidate-error-properties
branch
2 times, most recently
from
a550a44 to
aec740b
Compare
jerelmiller
force-pushed
the
jerel/separate-error-objects
branch
from
3ea9357 to
372a87b
Compare
src/link/retry/retryLink.ts
Outdated
| new CombinedProtocolErrors( | ||
| result.extensions[PROTOCOL_ERRORS_SYMBOL] | ||
| ) |
There was a problem hiding this comment.
Let's move this to the code where result.extensions[PROTOCOL_ERRORS_SYMBOL] is initially set, so that onError and code in/after QueryManager see the same error instance and not two different ones.
| // TODO: Determine if we still want the behavior of always rejecting | ||
| // "network" errors (any error that isn't a CombinedGraphQLErrors). |
There was a problem hiding this comment.
I would be so happy if we'd change this.
| error: new CombinedGraphQLErrors([ | ||
| new GraphQLError( | ||
| "homeWorld for character with ID 1000 could not be fetched.", | ||
| { path: ["hero", "heroFriends", 0, "homeWorld"] } | ||
| ), | ||
| ]), |
There was a problem hiding this comment.
| error: new CombinedGraphQLErrors([ | |
| new GraphQLError( | |
| "homeWorld for character with ID 1000 could not be fetched.", | |
| { path: ["hero", "heroFriends", 0, "homeWorld"] } | |
| ), | |
| ]), | |
| error: new CombinedGraphQLErrors([ | |
| { | |
| message: "homeWorld for character with ID 1000 could not be fetched.", | |
| path: ["hero", "heroFriends", 0, "homeWorld"] | |
| }, | |
| ]), |
Similar for other places here - these are not GraphQLError instances and we should probably not test for that.
| // TODO: Need to rewrite this to actually simulate a protocol error, not | ||
| // just a plain error. The underlying setup does not use the PROTOCOL_ERRORS_SYMBOL. |
There was a problem hiding this comment.
Let's do this as part of this PR.
| */ | ||
| // eslint-disable-next-line @typescript-eslint/no-restricted-types | ||
| export type GraphQLErrors = ReadonlyArray<GraphQLError>; | ||
|
|
||
| export type NetworkError = Error | ServerParseError | ServerError | null; |
There was a problem hiding this comment.
I'd go for an ErrorLike since the Error interface and Error class of TS overlap, which feels wonky.
src/errors/UnknownError.ts
Outdated
| @@ -0,0 +1,8 @@ | |||
| export class UnknownError extends Error { | |||
| constructor(errorType: unknown) { | |||
| super("An error of unknown type occurred", { cause: errorType }); | |||
There was a problem hiding this comment.
| super("An error of unknown type occurred", { cause: errorType }); | |
| super("An error of unexpected shape occurred", { cause: errorType }); |
There was a problem hiding this comment.
I renamed this to UnconventionalError which I think better captures this situation and updated the error message to your suggestion :) ff284d1
| response, | ||
| getResult(), | ||
| `Response not successful: Received status code ${response.status}` | ||
| throw new ServerError( |
There was a problem hiding this comment.
I kinda want to use invariant here too, so our error message extraction could kick in. Maybe we can have an overload of invariant that allows to change the error class in some way?
There was a problem hiding this comment.
My only rebuttal against that is that I see invariant as more of a validation error against user input (options, values, etc.) where this is a non-successful response.
There was a problem hiding this comment.
Maybe we'll just have an additional check(condition, [constructor, ...additionalConstructorArgs], [errorMessage, ...errorMessageReplacements]) function?
It's not a problem to hook that up, it's just a shame we currently don't use the message extraction mechanism for those messages.
There was a problem hiding this comment.
Let's hit this up in a separate PR so we can look at this throughout the client and find other areas where we might be able to optimize.
jerelmiller
force-pushed
the
jerel/consolidate-error-properties
branch
from
d77e202 to
ad8137f
Compare
jerelmiller
force-pushed
the
jerel/separate-error-objects
branch
from
002c5d0 to
c3b0070
Compare
jerelmiller
force-pushed
the
jerel/separate-error-objects
branch
from
c3b0070 to
6264e51
Compare
Closes #12200
Completely replaces
ApolloErrorwith separate error types. The error type depends on the type of error encapsulated byApolloError:Network errors
Network errors are no longer wrapped by the client and are passed through as-is. This should help prevent issues such as the Kaspersky issue where it looked like Apollo Client was to blame since errors were shown as
ApolloError.GraphQL Errors
These are now encapsulated in a
CombinedGraphQLErrorstype. You can access the original error objects with theerrorsproperty.Protocol errors
Replaced with a
CombinedProtocolErrorstype. You can access the original error objects with theerrorsproperty.Client errors
These weren't really used by the client, so these have no replacement. All non-GraphQL and non-protocol errors are passed through unchanged, so these replace that.