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

feat: Support for retries on 2xx by allowing to provide custom onFulfilled interceptor #156

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sebacampos
Copy link

Context

As per discussed in #25 there's a need to be able configure retries on 2xx, and the library does not support that since all the retry logic is inside the onError handler

PR Idea

One way to implement this without changing too much the scope/goal of the library is allowing to provide an optional custom onFulfilled interceptor. The custom interceptor should throw axios.Cancel so that onError handler from this library is invoked. This way consumers can specify their own conditions to retry on 2xx. And if not specified, it would just use the default onFulfilled handler.

Inspired on -> https://stackoverflow.com/a/55879318

Usage example

const myAxiosInstance = axios.create();
myAxiosInstance.defaults.raxConfig = {
  instance: myAxiosInstance
};

// Define custom function to use as onFulfilled handler
const onSuccess = (res: AxiosResponse) => {
  const customCondition = true; // Define your own condition here.
  if (customCondition) {
    throw new axios.Cancel('Operation canceled during onFulfilled handler.'); // Cancel the request so that onError handler is invoked.
  } else {
    return res;
  }
};

const interceptorId = rax.attach(myAxiosInstance, onSuccess); // Provide the axios instance and the custom interceptor
const res = await myAxiosInstance.get('https://test.local');

@JustinBeckwith Would appreciate if you could have a look and evaluate this add to the library, since there's a lot of people out there needing axios retries on 2xx.

@sebacampos sebacampos changed the title Support for retries on 2xx by allowing to provide custom onFulfilled interceptor feat: Support for retries on 2xx by allowing to provide custom onFulfilled interceptor Aug 9, 2021
@sebacampos
Copy link
Author

I was trying unit test this, but I'm not sure what should be the proper way to do it. Everything I tried made the test suite fail because of the thrown axios.Cancel, and the onError handler is not being invoked. Adding one example here:

// eslint-disable-next-line no-restricted-properties
  it.only('should retry with conditions passed on onFulfilled callback', async () => {
    const scopes = [
      nock(url).get('/').reply(200, 'milk'),
      nock(url).get('/').reply(200, 'milk'),
      nock(url).get('/').reply(200, 'cookies'),
    ];

    const myAxios = axios.create();

    const onSuccess = (res: AxiosResponse) => {
      console.log({resData: res.data});
      if (res.data !== 'cookies') {
        throw new axios.Cancel('Operation canceled by the user.');
      } else {
        return res;
      }
    };

    const cfg: rax.RaxConfig = {
      url,
      raxConfig: {
        instance: myAxios,
        retry: 2,
        statusCodesToRetry: [[200, 200]],
        onRetryAttempt: err => {
          const cfg = rax.getConfig(err);
          console.warn(`Retry attempt #${cfg?.currentRetryAttempt}`);
        },
      },
    };

    interceptorId = rax.attach(myAxios, onSuccess);
    const res = await myAxios.get(url, cfg);
    assert.strictEqual(res.data, 'cookies');
    scopes.forEach(s => s.done());
  });

image

@raccoonback
Copy link

raccoonback commented Aug 13, 2021

@sebacampos

I think the test fails because onSuccess callback function will not be retried.
Specifically , It is at the time onSuccess callback function is called, all retry has been requested has been successful.

So no retries from throw new axios.Cancel('Operation canceled by the user.') error.

As a result, this test doesn't seem proper, it's better to check whether the onSuccess() callback function is called.

@sebacampos
Copy link
Author

Hey @raccoonback thanks for the input!

Are you saying the feature suggestion on the PR won't work or just that the test case seems inappropriate? Sorry didn't get that very well. I am not very familiar with the test tools used in this library, if you could provide an example on how to test it, that would be awesome.

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.

3 participants