-
Notifications
You must be signed in to change notification settings - Fork 565
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
useAPQ errors indefinitely if the cache is down #3204
Comments
Hi! Thank you for raising this concern! While I do agree that bubbling up the error as is to the client is not a good thing, I'm not sure throwing a I think that what we should to is actually handling errors in place, and throw the appropriate error, which are @ardatan What do you think ? |
I'm also not 100% sure if
However, I agree that in cases where the cache has an intermittent error and isnt down, that this isnt the best outcome. Maybe circuitbreaking or having an error handler decide would be better. I don't know if it matters too much overall though. Always sending |
I think it can be handled inside the current API. Not sure if this is the responsibility of the plugin. useAPQ({
store: {
get(key) {
try{
return someStorage.get(key);
} catch(e) {
console.error(`Error while fetching the operation: ${key}`, e);
return null;
}
},
set(key) {
try{
return someStorage.get(key);
} catch(e) {
console.error(`Error while saving the operation: ${key}`, e);
return null;
}
}
}
}) |
returning I think the behavior is confusing enough that if not explicitly handled, it'd be worth including the recommended implementation in the documentation. |
When PersistedQueryNotFound is thrown, the client will send the original query and the hash, then the server attempts to save it. Even if it fails, it will do nothing then the server will return the response. I didn't understand what is the loop here. As a user, you can handle the error by yourself as in this test. |
Ty. That makes sense. |
@jdolle Can you send a PR to document this practise in our documentation? |
Fixed by #3213 |
Describe the bug
If
store.get
throws an exception, then that error will be raised to clients which can cause clients to retry indefinitely (depending on client retry logic)Your Example Website or App
https://github.com/dotansimha/graphql-yoga/blob/main/packages/plugins/apq/src/index.ts#L91
Steps to Reproduce the Bug or Issue
call
useAPQ
with a cache implementation that throws anew Error()
on get and set.Expected behavior
throw a
PersistedQueryNotSupported
to indicate to the client that this feature is disabled for nowhttps://www.apollographql.com/docs/react/api/link/persisted-queries/#protocol
This should allow clients to intelligently fallback to sending the full operation body.
Screenshots or Videos
No response
Platform
@graphql-yoga/*
version(s): [e.g. 2.5.1]Additional context
No response
The text was updated successfully, but these errors were encountered: