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

Unable to override raxConfig using axios 0.19.0 and retry-axios 2.1.1 #90

Closed
mkiddTempus opened this issue Jan 31, 2020 · 9 comments · Fixed by #91
Closed

Unable to override raxConfig using axios 0.19.0 and retry-axios 2.1.1 #90

mkiddTempus opened this issue Jan 31, 2020 · 9 comments · Fixed by #91
Labels
blocked bug Something isn't working released

Comments

@mkiddTempus
Copy link

I created the following gist to demonstrate the inability to override the config beyond the defaults. Executing this script fails on first attempt and no retry is executed. Using default settings, as shown in method 1, I've seen the 3 retry attempts in the config but this gist does not have that working. Execution methods are in comments at the bottom of the file.

https://gist.github.com/mkiddTempus/4ce156382c1b654b34d44d1900a37d2b

@JustinBeckwith JustinBeckwith added the bug Something isn't working label Feb 2, 2020
@JustinBeckwith
Copy link
Owner

I can verify this is a legit bug. Looking into it!

@JustinBeckwith
Copy link
Owner

Ok - update - I added a test specifically for this scenario in #91, and can verify it does in fact fail. Thank you for the great reproduction steps!

The bad news is that I have no idea how to fix it. As of today, there is no way to pin raxConfig on a defaults object in the way you're trying to do it. You would have to include the raxConfig in each relevant request - I believe this happened sometime between axios 0.18 and 0.19, and I never had a test covering that specific case.

A little more color and detail from the pull request I submitted:

The issue here boils down to the way axios merges their default configuration along with request level configuration. The merging code goes through a named set of parameters, and only merges known properties. raxConfig is pinned on the config, so it is not in said named list. The offending code is here:
https://github.com/axios/axios/blob/master/lib/core/mergeConfig.js#L19

var valueFromConfig2Keys = ['url', 'method', 'params', 'data'];
  var mergeDeepPropertiesKeys = ['headers', 'auth', 'proxy'];
  var defaultToConfig2Keys = [
    'baseURL', 'url', 'transformRequest', 'transformResponse', 'paramsSerializer',
    'timeout', 'withCredentials', 'adapter', 'responseType', 'xsrfCookieName',
    'xsrfHeaderName', 'onUploadProgress', 'onDownloadProgress',
    'maxContentLength', 'validateStatus', 'maxRedirects', 'httpAgent',
    'httpsAgent', 'cancelToken', 'socketPath'
  ];

We would need to somehow add raxConfig to the list of known mergeDeepPropertiesKeys, which is in no way at all extensible today. I have no idea how to fix this.

@avindra
Copy link

avindra commented Feb 6, 2020

mergeConfig is a total hack. The code shouldn't exist and hopefully it gets refactored out of existence in a future axios release.

The only other option, if you have a critical production app, is to just patch it directly yourself and add raxConfig to that list.

patch-package makes this quite easy. The patch for axios 0.19.x w/ retry-axios 2.1.1 should look like :

diff --git a/node_modules/axios/lib/core/mergeConfig.js b/node_modules/axios/lib/core/mergeConfig.js
index 8e1ce61..769f2a7 100644
--- a/node_modules/axios/lib/core/mergeConfig.js
+++ b/node_modules/axios/lib/core/mergeConfig.js
@@ -22,7 +22,7 @@ module.exports = function mergeConfig(config1, config2) {
     'timeout', 'withCredentials', 'adapter', 'responseType', 'xsrfCookieName',
     'xsrfHeaderName', 'onUploadProgress', 'onDownloadProgress',
     'maxContentLength', 'validateStatus', 'maxRedirects', 'httpAgent',
-    'httpsAgent', 'cancelToken', 'socketPath'
+    'httpsAgent', 'cancelToken', 'socketPath', 'raxConfig',
   ];
 
   utils.forEach(valueFromConfig2Keys, function valueFromConfig2(prop) {

@ntsim
Copy link

ntsim commented Mar 12, 2020

Might I suggest just not hanging raxConfig from the Axios's configuration at all?

From my own fork of this from a little while back, it's entirely possible to write the API using the closure context to avoid needing it at all:

export function attachRetry(
  instance: AxiosInstance,
  retryConfig: Partial<RetryConfig> = {},
) {
    return instance.interceptors.response.use(
    res => res,
    (err: AxiosError) => {
      const config: RetryConfig = {
        header: 'X-Retry',
        httpMethodsToRetry: ['GET', 'HEAD', 'PUT', 'OPTIONS', 'DELETE'],
        noResponseRetries: 2,
        retry: 3,
        retryDelay: 100,
        statusCodesToRetry: [
          [100, 199],
          [429, 429],
          [500, 599],
        ],
        ...retryConfig,
      };

     err.config.headers[config.header] =
        err.config.headers[config.header] || 0;

     //... rest of code
}

One of the main things that I do differently is store the number of retry attempts on a header instead of on raxConfig. By default I called it something like X-Retry.

If you're interested in a PR, I might be able to send something forward.

@harrydema
Copy link

Any news? It is useless this library if you can not configure it

@mkiddTempus
Copy link
Author

@harrydema I would suggest writing an interceptor yourself to accomplish the retry that responds to passed in options. From the looks of things axios is going to remain an opinionated library.

@bdcarr
Copy link
Contributor

bdcarr commented Jul 17, 2020

It looks like axios has fixed this in their upcoming v0.20.0 release: axios/axios#2190

@sk-
Copy link

sk- commented Aug 19, 2020

I can confirm it's working with axios 0.20.0-0. However, you need to make sure you are passing instance in the config otherwise it wont' work. See #121

@github-actions
Copy link

🎉 This issue has been resolved in version 2.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants