Add cause field to ApolloError.#11902
Conversation
🦋 Changeset detectedLatest commit: 5e6a6b4 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 |
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| this.message = errorMessage || generateErrorMessage(this); | ||
| this.extraInfo = extraInfo; | ||
| this.cause = | ||
| [networkError, ...(clientErrors || []), ...(graphQLErrors || [])].find( |
There was a problem hiding this comment.
Should protocolErrors be included in this list?
There was a problem hiding this comment.
No, they are not Error objects, so no consumer of cause could actually handle them.
There was a problem hiding this comment.
Fun fact: nothing in our codebase actually populates clientErrors, so I was thinking about skipping that, too. But I found a bunch of userland code on GitHub that manually creates ApolloError instances with clientErrors. Not what we had in mind, but I didn't want to break them.
There was a problem hiding this comment.
According to MDN, it looks like it doesn't have to be an Error instance in order to use it:
Error messages written for human consumption may be inappropriate for machine parsing — since they're subject to rewording or punctuation changes that may break any existing parsing written to consume them. So when throwing an error from a function, as an alternative to a human-readable error message, you can instead provide the cause as structured data, for machine parsing.
function makeRSA(p, q) { if (!Number.isInteger(p) || !Number.isInteger(q)) { throw new Error("RSA key generation requires integer inputs.", { cause: { code: "NonInteger", values: [p, q] }, }); } if (!areCoprime(p, q)) { throw new Error("RSA key generation requires two co-prime integers.", { cause: { code: "NonCoprime", values: [p, q] }, }); } // rsa algorithm… }
Despite our types, I think that graphqlErrors can also just be an array of objects (i.e. the raw parsed json value from the GraphQL response), not just GraphQLError instances.
Should we be concerned about that?
There was a problem hiding this comment.
Should we be concerned about that?
Hmmm... I think it's more about an expected shape than anything.
From that perspective, protocolErrors could get interesting again - they don't have most of the fields of a normal Error, but they do have a message field that some logging solution could read.
If we add them back in, which priority would you prefer?
There was a problem hiding this comment.
IMO the checked order should be networkError, graphQLErrors, protocolErrors, clientErrors and find the first instance of one. Thanks for humoring me :)
jerelmiller
left a comment
There was a problem hiding this comment.
Looks great! Let's get this one in 3.11
There seems to be progress on h3js/h3#691, so I'm opening a PR on our side.
Generally, this is probably a good idea independently of what they do there.