Include missing paths in MaxRetryExceededError instances #874
Conversation
this will help developers understand data source and service failures better
lib/errors/MaxRetryExceededError.js
Outdated
| this.message = "The allowed number of retries have been exceeded."; | ||
| this.missingPaths = missingPaths || []; | ||
| this.stack = (new Error()).stack; |
There was a problem hiding this comment.
Since we're in the client this may not be possible, but is there something like captureStackTrace available across different browsers?
There was a problem hiding this comment.
To remain cross platform I think (new Error()).stack is the best that can be done here.
| @@ -12,17 +12,18 @@ var MaxRetryExceededError = require("./../../errors/MaxRetryExceededError"); | |||
| */ | |||
| module.exports = function setRequestCycle(model, observer, groups, | |||
| isJSONGraph, isProgressive, count) { | |||
| var requestedAndOptimizedPaths = setGroupsIntoCache(model, groups); | |||
| var optimizedPaths = requestedAndOptimizedPaths.optimizedPaths; | |||
| var requestedPaths = requestedAndOptimizedPaths.requestedPaths; | |||
There was a problem hiding this comment.
Something to consider which could be a little scary: we're going from conditionally setting groups into cache, to always setting. It might be that the shape of what is set makes this inconsequential, but could be worth a close look before you merge.
There was a problem hiding this comment.
After discussion with Jafar and Steve, it seems logical to add any potential results returned from server into cache, even if it was on the last retried request.
Good catch though!
instead of missingPaths (requested)
and better tests for get
|
LGTM |
this will help developers understand data source and service failures better