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

issue#2609 | Sasha | predictable axios requests #2702

Merged
merged 7 commits into from Mar 1, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 28 additions & 0 deletions README.md
Expand Up @@ -530,6 +530,34 @@ const instance = axios.create();
instance.interceptors.request.use(function () {/*...*/});
```

When you add request interceptors, they are presumed to be asynchronous by default. This can cause a delay
in the execution of your axios request when the main thread is blocked (a promise is created under the hood for
the interceptor and your request gets put on the bottom of the call stack). If your request interceptors are synchronous you can add a flag
to the options object that will tell axios to run the code synchronously and avoid any delays in request execution.

```js
axios.interceptors.request.use(function (config) {
config.headers.test = 'I am only a header!';
return config;
}, null, { synchronous: true });
```

If you want to execute a particular interceptor based on a runtime check,
you can add a `runWhen` function to the options object. The interceptor will not be executed if the return
chinesedfan marked this conversation as resolved.
Show resolved Hide resolved
of `runWhen` is `false`. The function will be called with the config
object (don't forget that you can bind your own arguments to it as well.) This can be handy when you have an
asynchronous request interceptor that only needs to run at certain times.

```js
function onGetCall(config) {
return config.method === 'get';
}
axios.interceptors.request.use(function (config) {
config.headers.test = 'special get headers';
return config;
}, null, { runWhen: onGetCall });
```
chinesedfan marked this conversation as resolved.
Show resolved Hide resolved

## Handling Errors

```js
Expand Down
57 changes: 50 additions & 7 deletions lib/core/Axios.js
Expand Up @@ -45,20 +45,63 @@ Axios.prototype.request = function request(config) {
config.method = 'get';
}

// Hook up interceptors middleware
var chain = [dispatchRequest, undefined];
var promise = Promise.resolve(config);
var requestCancelled = config.cancelToken && config.cancelToken.reason;

// filter out skipped interceptors
var requestInterceptorChain = [];
var synchronousRequestInterceptors = true;
this.interceptors.request.forEach(function unshiftRequestInterceptors(interceptor) {
chain.unshift(interceptor.fulfilled, interceptor.rejected);
if (typeof interceptor.runWhen === 'function' && interceptor.runWhen(config) === false) {
chinesedfan marked this conversation as resolved.
Show resolved Hide resolved
return;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As runWhen is executed immediately, we need to try/catch of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused on this one. What do we want to do in the catch if the runWhen function fails? The function is provided by the consumer so we don't know what kind of errors it might throw. I would think we just want the program to fail at that point.

The axios configuration accepts other consumer provided functions such as validateStatus and paramsSerializer. Calls to those functions are not wrapped in try/catch blocks.


synchronousRequestInterceptors = synchronousRequestInterceptors && interceptor.synchronous;

requestInterceptorChain.push(interceptor.fulfilled, interceptor.rejected);
});
chinesedfan marked this conversation as resolved.
Show resolved Hide resolved

var responseInterceptorChain = [];
this.interceptors.response.forEach(function pushResponseInterceptors(interceptor) {
chain.push(interceptor.fulfilled, interceptor.rejected);
responseInterceptorChain.push(interceptor.fulfilled, interceptor.rejected);
});

while (chain.length) {
promise = promise.then(chain.shift(), chain.shift());
var promise;

if (requestCancelled || !synchronousRequestInterceptors) {
var chain = [dispatchRequest, undefined];

Array.prototype.unshift.apply(chain, requestInterceptorChain);
chain.concat(responseInterceptorChain);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chinesedfan @SashaKoro Here is the bug of issue #4036! The concat method does not change the existing arrays, but instead returns a new array. It cause response interceptors not append into chain!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it has been fixed in #4013 and released in v0.21.3.


promise = Promise.resolve(config);
while (chain.length) {
promise = promise.then(chain.shift(), chain.shift());
}

return promise;
}


var newConfig = config;
while (requestInterceptorChain.length) {
var onFulfilled = requestInterceptorChain.shift();
var onRejected = requestInterceptorChain.shift();
try {
newConfig = onFulfilled(newConfig);
} catch (error) {
onRejected(error);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If onRejected had solved the error, we should continue the loop. If not, should drop to the next onRejected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this would make the functionality different from the asynchronous behavior then.

When we have async interceptors we do a promise chain:

promise = promise.then(chain.shift(), chain.shift());

When any interceptor throws in a chained promise, the whole thing errors out.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SashaKoro Sorry for my long long latency. I mean,

var f = function(name) {
    return function(...args) {
        console.log(name, args)
    }
}

Promise.reject('bad')
     .then(f('f1'), f('f2'))
     .then(f('f3'), f('f4'))

// outputs
f2 [ 'bad' ]
f3 [ undefined ]

Even though the chain has an error, but f2 can resolve it and make the loop go on.

break;
}
}

try {
promise = dispatchRequest(newConfig);
} catch (error) {
throw error;
chinesedfan marked this conversation as resolved.
Show resolved Hide resolved
}

while (responseInterceptorChain.length) {
promise = promise.then(responseInterceptorChain.shift(), responseInterceptorChain.shift());
}

return promise;
Expand Down
6 changes: 4 additions & 2 deletions lib/core/InterceptorManager.js
Expand Up @@ -14,10 +14,12 @@ function InterceptorManager() {
*
* @return {Number} An ID used to remove interceptor later
*/
InterceptorManager.prototype.use = function use(fulfilled, rejected) {
InterceptorManager.prototype.use = function use(fulfilled, rejected, options) {
this.handlers.push({
fulfilled: fulfilled,
rejected: rejected
rejected: rejected,
synchronous: options ? options.synchronous : false,
runWhen: options ? options.runWhen : null
});
return this.handlers.length - 1;
};
Expand Down
15 changes: 8 additions & 7 deletions test/specs/adapter.spec.js
Expand Up @@ -2,18 +2,19 @@ var axios = require('../../index');

describe('adapter', function () {
it('should support custom adapter', function (done) {
var called = false;

axios('/foo', {
adapter: function (config) {
called = true;
adapter: function(config) {
return new Promise(function (resolve) {
setTimeout(function () {
config.headers.async = 'promise';
resolve(config);
}, 100);
});
}
});
}).catch(console.log);

setTimeout(function () {
expect(called).toBe(true);
chinesedfan marked this conversation as resolved.
Show resolved Hide resolved
done();
}, 100);
});
});

168 changes: 163 additions & 5 deletions test/specs/interceptors.spec.js
Expand Up @@ -9,7 +9,8 @@ describe('interceptors', function () {
axios.interceptors.response.handlers = [];
});

it('should add a request interceptor', function (done) {
it('should add a request interceptor (asynchronous by default)', function (done) {
var promiseResolveSpy = spyOn(window.Promise, 'resolve').and.callThrough();
axios.interceptors.request.use(function (config) {
config.headers.test = 'added by interceptor';
return config;
Expand All @@ -18,16 +19,148 @@ describe('interceptors', function () {
axios('/foo');

getAjaxRequest().then(function (request) {
request.respondWith({
status: 200,
responseText: 'OK'
});
expect(promiseResolveSpy).toHaveBeenCalled();
expect(request.requestHeaders.test).toBe('added by interceptor');
done();
});
});

it('should add a request interceptor (explicitly flagged as asynchronous)', function (done) {
var promiseResolveSpy = spyOn(window.Promise, 'resolve').and.callThrough();
axios.interceptors.request.use(function (config) {
config.headers.test = 'added by interceptor';
return config;
}, null, { synchronous: false });

axios('/foo');

getAjaxRequest().then(function (request) {
expect(promiseResolveSpy).toHaveBeenCalled();
expect(request.requestHeaders.test).toBe('added by interceptor');
done();
});
});

it('should add a request interceptor that is executed synchronously when flag is provided', function (done) {
var promiseResolveSpy = spyOn(window.Promise, 'resolve').and.callThrough();
axios.interceptors.request.use(function (config) {
config.headers.test = 'added by synchronous interceptor';
return config;
}, null, { synchronous: true });

axios('/foo');

getAjaxRequest().then(function (request) {
expect(promiseResolveSpy).not.toHaveBeenCalled();
expect(request.requestHeaders.test).toBe('added by synchronous interceptor');
done();
});
});

it('should execute asynchronously when not all interceptors are explicitly flagged as synchronous', function (done) {
var promiseResolveSpy = spyOn(window.Promise, 'resolve').and.callThrough();
axios.interceptors.request.use(function (config) {
config.headers.foo = 'uh oh, async';
return config;
});

axios.interceptors.request.use(function (config) {
config.headers.test = 'added by synchronous interceptor';
return config;
}, null, { synchronous: true });

axios.interceptors.request.use(function (config) {
config.headers.test = 'uh oh, async also';
return config;
});

axios('/foo');

getAjaxRequest().then(function (request) {
expect(promiseResolveSpy).toHaveBeenCalled();
expect(request.requestHeaders.foo).toBe('uh oh, async');
expect(request.requestHeaders.test).toBe('uh oh, async also');
done();
});
});

it('runs the interceptor if runWhen function is provided and resolves to true', function (done) {
function onGetCall(config) {
return config.method === 'get';
}
axios.interceptors.request.use(function (config) {
config.headers.test = 'special get headers';
return config;
}, null, { runWhen: onGetCall });

axios('/foo');

getAjaxRequest().then(function (request) {
expect(request.requestHeaders.test).toBe('special get headers');
done();
});
});

it('does not run the interceptor if runWhen function is provided and resolves to false', function (done) {
function onPostCall(config) {
return config.method === 'post';
}
axios.interceptors.request.use(function (config) {
config.headers.test = 'special get headers';
return config;
}, null, { runWhen: onPostCall });

axios('/foo');

getAjaxRequest().then(function (request) {
expect(request.requestHeaders.test).toBeUndefined()
done();
});
});

it('does not run async interceptor if runWhen function is provided and resolves to false (and run synchronously)', function (done) {
var promiseResolveSpy = spyOn(window.Promise, 'resolve').and.callThrough();

function onPostCall(config) {
return config.method === 'post';
}
axios.interceptors.request.use(function (config) {
config.headers.test = 'special get headers';
return config;
}, null, { synchronous: false, runWhen: onPostCall });

axios.interceptors.request.use(function (config) {
config.headers.sync = 'hello world';
return config;
}, null, { synchronous: true });

axios('/foo');

getAjaxRequest().then(function (request) {
expect(promiseResolveSpy).not.toHaveBeenCalled()
expect(request.requestHeaders.test).toBeUndefined()
expect(request.requestHeaders.sync).toBe('hello world')
done();
});
});

it('should add a request interceptor with an onRejected block that is called if interceptor code fails', function (done) {
var rejectedSpy = jasmine.createSpy('rejectedSpy');
var error = new Error('deadly error');
axios.interceptors.request.use(function () {
throw error;
}, function() {
rejectedSpy(error);
chinesedfan marked this conversation as resolved.
Show resolved Hide resolved
}, { synchronous: true });

axios('/foo');

getAjaxRequest().then(function () {
expect(rejectedSpy).toHaveBeenCalledWith(error);
done();
});
});

it('should add a request interceptor that returns a new config object', function (done) {
axios.interceptors.request.use(function () {
return {
Expand Down Expand Up @@ -237,6 +370,31 @@ describe('interceptors', function () {
});
});

it('should remove async interceptor before making request and execute synchronously', function (done) {
var promiseResolveSpy = spyOn(window.Promise, 'resolve').and.callThrough();
var asyncIntercept = axios.interceptors.request.use(function (config) {
config.headers.async = 'async it!';
return config;
}, null, { synchronous: false });

var syncIntercept = axios.interceptors.request.use(function (config) {
config.headers.sync = 'hello world';
return config;
}, null, { synchronous: true });


axios.interceptors.request.eject(asyncIntercept);

axios('/foo')

getAjaxRequest().then(function (request) {
expect(promiseResolveSpy).not.toHaveBeenCalled();
expect(request.requestHeaders.async).toBeUndefined();
expect(request.requestHeaders.sync).toBe('hello world');
done()
});
});

it('should execute interceptors before transformers', function (done) {
axios.interceptors.request.use(function (config) {
config.data.baz = 'qux';
Expand Down