Start extracting service policy boilerplate to common function #5141
Conversation
| it('has expected JavaScript exports', () => { | ||
| expect(Object.keys(allExports)).toMatchInlineSnapshot(` | ||
| Array [ | ||
| "createServicePolicy", |
There was a problem hiding this comment.
This line specifically refers to the new export we are adding; the rest of the exports already exist, we are just adding tests for all exports for the first time.
mcmire
force-pushed
the
extract-create-service-policy-1
branch
from
cb1b08e to
d23f319
Compare
We would like to use the Cockatiel library in our service classes to ensure that requests are retried using the circuit breaker pattern. Some of our service classes do this already, but we are copying and pasting the code around. This commit extracts some of the boilerplate code to a new function in the `@metamask/controller-utils` package, `createServicePolicy` so that we no longer have to do this. Because it would require an enormous amount of tests, this commit does not include a complete version of `createServicePolicy`, notably omitting `maxRetries` option as well as a way to handle slow requests; those will be added later.
mcmire
force-pushed
the
extract-create-service-policy-1
branch
from
d23f319 to
86ef36f
Compare
| maxAttempts: DEFAULT_MAX_RETRIES, | ||
| // Retries of the service will be executed following ever increasing delays, | ||
| // determined by a backoff formula. | ||
| backoff: new ExponentialBackoff(), |
There was a problem hiding this comment.
Nit: do we want to make this customizable but defaulting to ExponentialBackoff ?
| onBreak?: () => void; | ||
| onRetry?: () => void; | ||
| } = {}): IPolicy { | ||
| const retryPolicy = retry(handleAll, { |
There was a problem hiding this comment.
Just a question we are using handleAll which will retry any kind of failure ? do we want to filter some failures we don't want to retry ?
There was a problem hiding this comment.
Yes, good thought. I plan on adding an option to customize that in another PR.
| @@ -0,0 +1,589 @@ | |||
| import { useFakeTimers } from 'sinon'; | |||
There was a problem hiding this comment.
Nit: can't we just use jest fake timers ?
There was a problem hiding this comment.
No, because we're still using an older version of Jest, and it doesn't support the "async" varieties of next and tick.
Explanation
We would like to use the Cockatiel library in our service classes to ensure that requests are retried using the circuit breaker pattern. Some of our service classes do this already, but we are copying and pasting the code around.
This commit extracts some of the boilerplate code to a new function in the
@metamask/controller-utilspackage,createServicePolicy, so that we no longer have to do this. Because it would require an enormous amount of tests, this commit does not include a complete version ofcreateServicePolicy, notably omitting themaxRetriesoption as well as anonDegradedcallback so consumers can handle slow requests. Those will be added later.References
Progresses #4994.
Changelog
(Updated in PR.)
Checklist