[2.x] Failed config mutation in refreshAuthCall

[antonkomarev]

In v2 beta there is a code block where failed request is re-sending:

.then(() => {
    error.config.skipAuthRefresh = true;
    return axios(error.response.config);
});

I found an issue in this implementation. In my application refreshAuthCall method requests new access token and stores it in the local storage. New access token injects to the request config with request interceptor, but this interceptor is ignored in current implementation because new Axios instance without any interceptors is using for re-sending.

If initial Axios instance will be used in that case there is a recursion loop:

.then(() => {
    error.config.skipAuthRefresh = true;
    return instance(error.response.config);
});

I can see another one issue with using initial instance. Failed request config state could be already mutated and if we will pass it to the initial instance with all interceptors there could be raised another issue because we will try to mutate already mutated data. From this perspective current implementation seems okay.

But on the other hand if new Axios instance is used for re-sending request - then it wouldn't be handled by response interceptors which were used in initial Axios instance. And there will be issues with response handling.

Another one edge case, that I don't want to mutate failedRequest.response.config in refreshAuthCall method because it breaks single responsibility and this kind of object mutation could lead to unexpected behavior. With current implementation it will require additional property or option for preparing request for re-sending. If initial Axios instance will be used for re-sending - then this case will disappear.

Any ideas how to deal with it?

[Flyrell]

Hey, sorry for the late reply and thank you for all of your contributions.

You've raised a good point here when it comes to SRP, which I commented in #41. Unfortunately, I don't see a better solution as of right now.

Your second point, which is "mutating already mutated data"... great catch here! I haven't thought about it at all. I think passing optional "refreshing instance" as an option would do a thing for users that encounter this, but I'm not sure how it's ok when it comes to the software architecture principles. Maybe you can help as you have a greater understanding of them?

[Flyrell]

Added and merged option to specify the retryInstance in #66.