-
Notifications
You must be signed in to change notification settings - Fork 62
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
feat: add request and response interceptors #619
Conversation
@@ -38,7 +38,7 @@ | |||
"license": "Apache-2.0", | |||
"devDependencies": { | |||
"@babel/plugin-proposal-private-methods": "^7.18.6", | |||
"@compodoc/compodoc": "^1.1.9", | |||
"@compodoc/compodoc": "1.1.21", |
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.
Pinning compodoc because the downstream dep @angular-devkit/schematics
appears to have dropped < Node 16 in the latest version which was breaking CI for us.
src/gaxios.ts
Outdated
* | ||
* @returns {Promise<T>} Promise that resolves to the set of options or response after interceptors are applied. | ||
*/ | ||
async #applyInterceptors< |
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.
Rather than adding GaxiosInterceptorType
perhaps we can have separate methods for #applyInterceptors
; #applyRequestInterceptors
and #applyResponseInterceptors
- the logic in #applyInterceptors
can be cleanly separated by its if/else
condition
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.
That was what I was originally trying to avoid but now that I think about it, makes more sense, single responsibility and all that. Will adjust.
src/interceptor.ts
Outdated
/** | ||
* Class to manage collections of GaxiosInterceptors for both requests and responses. | ||
*/ | ||
export class GaxiosInterceptorManager<T extends GaxiosOptions | GaxiosResponse> |
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.
What are the benefits of GaxiosInterceptorManager
over a Map
? They are also ordered by insertion.
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 keep forgetting that JavaScript maps behave like 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.
The additional benefit of using a class is that we don't have to add functions to Gaxios.ts
to add, remove, clear interceptors. It keeps things a bit more separated that way.
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.
Simplified the class to use maps under the hood. Kept it wrapped in a class just to keep the logic from seeping into Gaxios.ts
}, | ||
}); | ||
// Because the options wind up being invalid the call will reject with a URL problem. | ||
assert.rejects(instance.request({url})); |
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.
Do we need an await
here if we make the calls between request
and the interceptor calls async?
assert.rejects(instance.request({url})); | |
assert.rejects(await instance.request({url})); |
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 don't believe so assert.rejects
will await the promise returning function.
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! One optional suggestion
src/interceptor.ts
Outdated
export class GaxiosInterceptorManager< | ||
T extends GaxiosOptions | GaxiosResponse, | ||
> { | ||
interceptorMap: Map<Number, GaxiosInterceptor<T> | null>; | ||
|
||
constructor() { | ||
this.interceptorMap = new Map<Number, GaxiosInterceptor<T>>(); | ||
} | ||
|
||
/** | ||
* Adds an interceptor. | ||
* | ||
* @param {GaxiosInterceptor} interceptor the interceptor to be added. | ||
* | ||
* @returns {number} an identifier that can be used to remove the interceptor. | ||
*/ | ||
addInterceptor(interceptor: GaxiosInterceptor<T>): number { | ||
const index = this.interceptorMap.size; | ||
this.interceptorMap.set(index, interceptor); | ||
|
||
return index; | ||
} | ||
|
||
/** | ||
* Removes an interceptor. | ||
* | ||
* @param {number} id the previously id of the interceptor to remove. | ||
*/ | ||
removeInterceptor(id: number) { | ||
if (this.interceptorMap.has(id)) { | ||
this.interceptorMap.set(id, null); | ||
} | ||
} | ||
|
||
/** | ||
* Removes all interceptors. | ||
*/ | ||
removeAll() { | ||
this.interceptorMap.clear(); | ||
} | ||
} |
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.
[completely optional]: Actually, I'm realizing a Set
might be better here than a Map
. Set
s are also kept in insertion order and the API is 1:1:
addInterceptor()
->add()
removeInterceptor(number)
->delete(function)
(might be easier for customers to find/remove)removeAll
->clear
export class GaxiosInterceptorManager< | |
T extends GaxiosOptions | GaxiosResponse, | |
> { | |
interceptorMap: Map<Number, GaxiosInterceptor<T> | null>; | |
constructor() { | |
this.interceptorMap = new Map<Number, GaxiosInterceptor<T>>(); | |
} | |
/** | |
* Adds an interceptor. | |
* | |
* @param {GaxiosInterceptor} interceptor the interceptor to be added. | |
* | |
* @returns {number} an identifier that can be used to remove the interceptor. | |
*/ | |
addInterceptor(interceptor: GaxiosInterceptor<T>): number { | |
const index = this.interceptorMap.size; | |
this.interceptorMap.set(index, interceptor); | |
return index; | |
} | |
/** | |
* Removes an interceptor. | |
* | |
* @param {number} id the previously id of the interceptor to remove. | |
*/ | |
removeInterceptor(id: number) { | |
if (this.interceptorMap.has(id)) { | |
this.interceptorMap.set(id, null); | |
} | |
} | |
/** | |
* Removes all interceptors. | |
*/ | |
removeAll() { | |
this.interceptorMap.clear(); | |
} | |
} | |
export class GaxiosInterceptorManager< | |
T extends GaxiosOptions | GaxiosResponse, | |
> extends Set<GaxiosInterceptor<T> | null> {} |
And as feats are added to the Set
we can get new stuff for free, like this handy difference
method:
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #490 🦕