-
Notifications
You must be signed in to change notification settings - Fork 100
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
pre-request and post-response interceptors are added #24
base: master
Are you sure you want to change the base?
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
Hi @Arminkhodaei! This looks really great. I think I may need to "golf" the implementation size down a fair bit though - is it okay if I do that in a PR targeting your implementation here? |
Sure, feel free to do so :) |
@developit hate to be annoying, but any progress here? Imagine this would amount to a drop-in axios replacement for a lot of folks if this functionality was added. |
Hiya @swrobel - apologies, this is the last feature PR I have left to merge before cutting the next release and it needs a lot of golfing to land without bloating the size to the point where it's too close to Axios' size to matter. For context, here's the size bot's output (doesn't work on forks 🤒): Size Change: +573 B (17%) Total Size: 3.24 kB
|
redaxios.interceptors = { | ||
request: new Interceptor(), | ||
response: new Interceptor() | ||
}; |
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.
Using an imported class here adds a bunch of module and setup overhead and makes it so the registry of interceptors can't be a simple local variable.
/** | ||
* @type {Array<{done: Function, error: Function }>} | ||
*/ | ||
this.handlers = []; |
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.
This doesn't need a pretty name, it could be this._ = []
this.handlers.push({ | ||
done, | ||
error: error || (() => {}) | ||
}); | ||
|
||
return this.handlers.length - 1; |
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.
push() returns the array's new length:
this.handlers.push({ | |
done, | |
error: error || (() => {}) | |
}); | |
return this.handlers.length - 1; | |
return this.handlers.push({ done, error }) - 1; |
Also done
and error
could just be Array indices, or even the arguments object:
this.use = function() {
return this.handlers.push(arguments) - 1;
}
if (this.handlers[id]) | ||
this.handlers[id] = null; |
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.
No harm in re-assigning a null value to null again:
if (this.handlers[id]) | |
this.handlers[id] = null; | |
this.handlers[id] = null; |
I'm not going to have time to get this golfed tonight, but in order to get this merged the interceptors need to be stored in a local variable and have no intermediary properties (an array of arrays maybe). Also, looking at the Axios docs makes me think this may actually be slightly different from how Axios works. I need to look into it, since I've actually never used axios, but I'm thinking it might be possible to use a |
Apologies for the long review on this one. The golfing took too long and I decided to cut a 0.3.0 release with everything except interceptors. We'll get it merged, I need to figure out where the heck I had my WIP byte-shaving PR... |
How much size will this add? Would it be appropriate to create separate versions of this library (in the form of separate exports) to allow for users to pull in the smallest needed slice of axios compat? |
I'm also interested in switching to redaxios, but interceptor support is a must :( |
Any news on this? |
Bumping this, we would really like to use redaxios but interceptors are quite important for us :) |
@developit is interceptors part of redaxios yet? I didn't know about redaxios, so I tried my own initially yarn add onl which has interceptors. I would love to switch to redaxios. Better name and likely better tested but interceptors is really a must have. I can see how the focus on size is important to this lib. That said, I feel that the size increase in this case would be totally acceptable and a lot of potential users would make the switch when interceptors are added. |
It still doesn't merged? unbelievable |
any updates on this? |
up! |
@developit any updates? I would really like to start using redaxios in all my projects, but interceptors are quite important for my use. |
Hey guys, saw this is like the posible way to resolve the problem of axios. May i know what happen here? |
@Arios509 been 1.5 years since he last said he'd get it merged, so I doubt it's happening. Better to look for alternatives. Just FYI, you can do with Fetch whatever you can do with Axios/redaxios and it's a platform standard, so perhaps better to use that instead. We actually wrote a wrapper around fetch to make certain tasks easier like caching requests (and busting the cache), adding interceptors, automatically stringifying/parsing JSON, stuff like that: https://lion-web.netlify.app/docs/tools/ajax/overview/ |
Thats good to know, i am looking for a stable replacement too. I will look into it and see how should i update for my project if suitable. Thanks. At the same time i have use the #24 and update in the latest code which is this #75, this is just to solve my problem. |
Request interceptors are not just about catching errors. In there, you can do things like manipulate configuration before it's sent. In the case of reject, it can also be something related to configurations be it parsing or bad syntax. Redaxios can have both if you ask me. @developit I understand you wish to keep package size to as little as possible, so here is what could also be done. This way, people will install and use only what they really need. |
According to the #9 issue,
I've tried to keep it as concise as possible. Hopefully, it would be useful.