Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add request and response interceptors #619

Merged
merged 6 commits into from May 15, 2024

Conversation

ddelgrosso1
Copy link
Contributor

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:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #490 馃

@ddelgrosso1 ddelgrosso1 requested a review from a team as a code owner May 2, 2024 18:31
@product-auto-label product-auto-label bot added the size: l Pull request size is large. label May 2, 2024
@@ -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",
Copy link
Contributor Author

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.

@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 2, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 2, 2024
src/gaxios.ts Outdated
*
* @returns {Promise<T>} Promise that resolves to the set of options or response after interceptors are applied.
*/
async #applyInterceptors<
Copy link
Member

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

Copy link
Contributor Author

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.

/**
* Class to manage collections of GaxiosInterceptors for both requests and responses.
*/
export class GaxiosInterceptorManager<T extends GaxiosOptions | GaxiosResponse>
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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}));
Copy link
Member

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?

Suggested change
assert.rejects(instance.request({url}));
assert.rejects(await instance.request({url}));

Copy link
Contributor Author

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.

@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 3, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 3, 2024
@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 8, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 8, 2024
Copy link
Member

@danielbankhead danielbankhead left a 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

Comment on lines 39 to 79
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();
}
}
Copy link
Member

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. Sets 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
Suggested change
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:

@ddelgrosso1 ddelgrosso1 added the owlbot:run Add this label to trigger the Owlbot post processor. label May 14, 2024
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label May 14, 2024
@ddelgrosso1 ddelgrosso1 merged commit 059fe77 into googleapis:main May 15, 2024
15 checks passed
@ddelgrosso1 ddelgrosso1 deleted the interceptors branch May 15, 2024 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interceptors
2 participants