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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pre-request and post-response interceptors are added #24

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Arminkhodaei
Copy link
Contributor

According to the #9 issue,
I've tried to keep it as concise as possible. Hopefully, it would be useful.

@googlebot
Copy link
Collaborator

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 @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@Arminkhodaei
Copy link
Contributor Author

@googlebot I signed it!

@googlebot
Copy link
Collaborator

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@developit
Copy link
Owner

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?

@Arminkhodaei
Copy link
Contributor Author

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 :)

@swrobel
Copy link

swrobel commented Jul 14, 2020

@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.

@developit
Copy link
Owner

developit commented Jul 16, 2020

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

Filename Size Change
dist/redaxios.js 1.06 kB +192 B (18%) ⚠️
dist/redaxios.module.js 1.07 kB +192 B (18%) ⚠️
dist/redaxios.umd.js 1.11 kB +189 B (16%) ⚠️

compressed-size-action

Comment on lines +116 to +119
redaxios.interceptors = {
request: new Interceptor(),
response: new Interceptor()
};
Copy link
Owner

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 = [];
Copy link
Owner

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._ = []

Comment on lines +28 to +33
this.handlers.push({
done,
error: error || (() => {})
});

return this.handlers.length - 1;
Copy link
Owner

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:

Suggested change
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;
}

Comment on lines +40 to +41
if (this.handlers[id])
this.handlers[id] = null;
Copy link
Owner

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:

Suggested change
if (this.handlers[id])
this.handlers[id] = null;
this.handlers[id] = null;

@developit
Copy link
Owner

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 .then(interceptor) loop for response interceptors, and request error interceptors should just be ignored (since fetch cannot have request errors, only response errors).

@developit
Copy link
Owner

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...

@afleming-godaddy
Copy link

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?

@robjuz
Copy link

robjuz commented Jul 29, 2020

I'm also interested in switching to redaxios, but interceptor support is a must :(

@bsdis
Copy link

bsdis commented Sep 21, 2020

Any news on this?

@jorenbroekema
Copy link

Bumping this, we would really like to use redaxios but interceptors are quite important for us :)

@livinglogic-nl
Copy link

@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.

@fy0
Copy link

fy0 commented Apr 14, 2021

It still doesn't merged? unbelievable

@markpinero
Copy link

any updates on this?

@robjuz
Copy link

robjuz commented Oct 6, 2021

up!

@juliovedovatto
Copy link

@developit any updates?

I would really like to start using redaxios in all my projects, but interceptors are quite important for my use.

@Arios509
Copy link

Hey guys, saw this is like the posible way to resolve the problem of axios. May i know what happen here?

@jorenbroekema
Copy link

@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/

@Arios509
Copy link

@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.

@CatchABus
Copy link

CatchABus commented Nov 21, 2022

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 .then(interceptor) loop for response interceptors, and request error interceptors should just be ignored (since fetch cannot have request errors, only response errors).

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.
Convert this repo into a monorepo and split redaxios into modules. Current version can be redaxios-core with support of hooks for rest of modules to work, then have a redaxios-interceptors package for interceptors and so on.

This way, people will install and use only what they really need.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.