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 5 commits
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 and only if** the return
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: 49 additions & 8 deletions lib/core/Axios.js
Expand Up @@ -45,20 +45,61 @@ Axios.prototype.request = function request(config) {
config.method = 'get';
}

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

// 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.unshift(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 (!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) {
return Promise.reject(error);
}

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
93 changes: 86 additions & 7 deletions test/specs/adapter.spec.js
@@ -1,19 +1,98 @@
var axios = require('../../index');

describe('adapter', function () {
beforeEach(function () {
jasmine.Ajax.install();
});

afterEach(function () {
jasmine.Ajax.uninstall();
});

it('should support custom adapter', function (done) {
var called = false;
axios('/foo', {
adapter: function barAdapter(config) {
return new Promise(function dispatchXhrRequest(resolve) {
var request = new XMLHttpRequest();
request.open('GET', '/bar');

request.onreadystatechange = function () {
resolve({
config: config,
request: request
});
};

request.send(null);
});
}
}).catch(console.log);

getAjaxRequest().then(function(request) {
expect(request.url).toBe('/bar');
done();
});
});

it('should execute adapter code synchronously', function (done) {
var asyncFlag = false;
axios('/foo', {
adapter: function (config) {
called = true;
adapter: function barAdapter(config) {
return new Promise(function dispatchXhrRequest(resolve) {
var request = new XMLHttpRequest();
request.open('GET', '/bar');

request.onreadystatechange = function () {
resolve({
config: config,
request: request
});
};

expect(asyncFlag).toBe(false);
request.send(null);
});
}
}).catch(console.log);

asyncFlag = true;

getAjaxRequest().then(function() {
done();
});
});

it('should execute adapter code asynchronously when interceptor is present', function (done) {
var asyncFlag = false;

axios.interceptors.request.use(function (config) {
config.headers.async = 'async it!';
return config;
});

axios('/foo', {
adapter: function barAdapter(config) {
return new Promise(function dispatchXhrRequest(resolve) {
var request = new XMLHttpRequest();
request.open('GET', '/bar');

request.onreadystatechange = function () {
resolve({
config: config,
request: request
});
};

expect(asyncFlag).toBe(true);
request.send(null);
});
}
}).catch(console.log);

setTimeout(function () {
expect(called).toBe(true);
chinesedfan marked this conversation as resolved.
Show resolved Hide resolved
asyncFlag = true;

getAjaxRequest().then(function() {
done();
}, 100);
});
});
});