-
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
PersistedQueryLink: also check for extension error codes #10807
Conversation
🦋 Changeset detectedLatest commit: dd53bcc 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 |
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.
Looks great! So sorry this one slipped past us for so long.
Had a couple minor nits, but overall the PR looks good. Thanks for addressing!
size-limit report 📦
|
✅ Deploy Preview for apollo-client-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
export namespace PersistedQueryLink { | ||
interface BaseOptions { | ||
disable?: (error: ErrorResponse) => boolean; | ||
disable?: (error: ErrorResponse, errorsByMessageAndCode: ErrorsByMessageAndCode) => boolean; |
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.
@phryneas @jerelmiller If I understand correctly, ErrorResponse
is a type that basically literally is "the argument to this disable function" and isn't used for anything else. So it would seem reasonable to add errorsByMessage and errorsByCode directly to it rather than adding a second argument? The fact that the one instantiation of this type is called disablePayload
seems telling...
if (errorMessages.PersistedQueryNotSupported) { | ||
if ( | ||
byMessage.PersistedQueryNotSupported || | ||
byCode[PERSISTED_QUERY_NOT_SUPPORTED] |
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.
not clear why byMessage is using .
here and byCode is indirecting through a const
and using []
but 🤷
I see both of my comments are addressed in #10806 :) |
This one fixes #10253
Checklist: