-
Notifications
You must be signed in to change notification settings - Fork 2.7k
Add static is methods to error types for type narrowing
#12546
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
Conversation
|
commit: |
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
size-limit report 📦
|
src/errors/CombinedGraphQLErrors.ts
Outdated
| /** Determine if an error is a `CombinedGraphQLErrors` instance */ | ||
| static is(error: unknown): error is CombinedGraphQLErrors { | ||
| return ( | ||
| error instanceof CombinedGraphQLErrors || |
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 figure its best to try instanceof first since its harder to spoof. If that fails for whatever reason (multiple versions of the library for example), then we use the lower fidelity check on error.name to see if it matches.
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 understand the thought, but "harder to spoof" doesn't seem like a valid argument here: if someone were to spoof it, they could just set the name and the next check would still succeed - no matter if we optionally check for instanceof first or not.
If we want to prevent spoofing, we could use the mechanism that Error.isErrorand Array.isArray use:
Create a property with a well-known symbol key and check for that. (The native solutions use a [[ErrorData]] internal engine slot for that, which we obviously cannot do.)
Really not sure if that's worth it, though, and it might make serialization harder.
As it is right now, I believe the instanceof would only be beneficial if someone had a valid CombinedGraphQLErrors instance, but changed the value of name manually - and I'm not sure if we should waste bundle size on supporting that.
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.
Check Symbol.for('apollo.error') property and compare name
phryneas
left a comment
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.
Setting this as "Request Changes" so we don't forget discussing the instanceof check.
🦋 Changeset detectedLatest commit: e4bb1e8 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 |
| const unconventionalError = new UnconventionalError(Symbol()); | ||
|
|
||
| test("CombinedGraphQLErrors.is", () => { | ||
| expect(CombinedGraphQLErrors.is(graphQLErrors)).toBe(true); |
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.
Would be nice to test this with structuredClone to simulate an error from another realm, but we are polyfilling structuredClone with JSON.parse/JSON.stringify here so that won't work.
a7e2a4d to
88e2954
Compare
Followup to #12539 (comment)
Make it easier to type-narrow an error type by adding static
ismethods to each error type. This is a bit more robust than just usinginstanceofsince that might fail due to e.g. multiple versions of Apollo Client installed.