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

Conversation

@SashaKoro
Copy link
Contributor

@SashaKoro SashaKoro commented Jan 30, 2020

  • axios requests are not delayed by pre-emptive promise creation by default
  • add options to interceptors api ("synchronous" and "runWhen"
  • add documentation and unit tests
@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Jan 30, 2020

Issue for context: #2609

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Jan 30, 2020

@chinesedfan Let me know if anything is unclear or needs some extra work. Happy to make changes. 😃

Loading

Copy link
Collaborator

@chinesedfan chinesedfan left a comment

@SashaKoro Really thanks for your work. Left some comments to discuss.

  • Shall we consider the response interceptors someday? How about call as synchronousRequest and requestWhen?
  • In fact, runWhen can be achieved by adding a conditional return statement in the beginning of interceptors. I want to make axios as simple as possible.
  • No need to update dist files, because we will update them together before releasing.

Loading

README.md Show resolved Hide resolved
Loading
lib/core/Axios.js Outdated Show resolved Hide resolved
Loading
lib/core/Axios.js Show resolved Hide resolved
Loading
lib/core/Axios.js Show resolved Hide resolved
Loading
lib/core/Axios.js Outdated Show resolved Hide resolved
Loading
lib/core/Axios.js Outdated Show resolved Hide resolved
Loading
lib/core/Axios.js Outdated Show resolved Hide resolved
Loading
test/specs/adapter.spec.js Outdated Show resolved Hide resolved
Loading
test/specs/interceptors.spec.js Outdated Show resolved Hide resolved
Loading
test/specs/interceptors.spec.js Outdated Show resolved Hide resolved
Loading
@chinesedfan chinesedfan requested a review from yasuf Jan 31, 2020
@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Jan 31, 2020

@chinesedfan Thanks for the feedback! I will be working through the comments, responding and updating the MR over the next couple of days.

To answer the question of:

"How about call as synchronousRequest and requestWhen?"

I explicitly did not use request in the naming because the interceptors do not always fire requests inside of themselves. Sometimes they are just updating the configuration object, doing logging or updating UI state.

Loading

@chinesedfan
Copy link
Collaborator

@chinesedfan chinesedfan commented Feb 1, 2020

I explicitly did not use request in the naming because the interceptors do not always fire requests inside of themselves. Sometimes they are just updating the configuration object, doing logging or updating UI state.

@SashaKoro My fault. axios.interceptors.request/response are both instances of InterceptorManager. And I have replied to other questions for you.

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Feb 5, 2020

To answer:

"In fact, runWhen can be achieved by adding a conditional return statement in the beginning of interceptors."

I put runWhen as a separate config option because axios needs to know if the interceptor will run before its executed. This is in the case where asynchronous interceptors that do not need to run can be filtered out prior to going into sync or async flows.

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Feb 5, 2020

To answer:

Shall we consider the response interceptors someday?

Yeah, that's a good call. Do you mind if I look at that as part of a separate MR? Just to wrap this one up (because that story could have its own unknowns).

Loading

- axios requests are not delayed by pre-emptive promise creation by default
- add options to interceptors api ("synchronous" and "runWhen")
- add documentation and unit tests
@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Feb 5, 2020

@chinesedfan I've updated the pull request with the changes requested.

Loading

Copy link
Collaborator

@chinesedfan chinesedfan left a comment

@SashaKoro Resolved or replied old conversations. And left new comments for your updates.

Do you mind if I look at that as part of a separate MR? Just to wrap this one up (because that story could have its own unknowns).

Sure. I think less people care about that.

Loading

README.md Outdated Show resolved Hide resolved
Loading
lib/core/Axios.js Outdated Show resolved Hide resolved
Loading
test/specs/interceptors.spec.js Outdated Show resolved Hide resolved
Loading
test/specs/adapter.spec.js Show resolved Hide resolved
Loading
@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Feb 7, 2020

@chinesedfan Pull request updated with requested changes

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Feb 13, 2020

@chinesedfan I will address your latest comments soon. This week has been pretty busy.

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Feb 14, 2020

@chinesedfan Pull request updated with requested changes.

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Feb 18, 2020

@chinesedfan Pull request updated with the new unit tests and the removed requestCancelled check.

Loading

Copy link
Collaborator

@chinesedfan chinesedfan left a comment

@SashaKoro No other reviewers now. After days, I checked this pull request again and found 2 new points.

Loading

chain.unshift(interceptor.fulfilled, interceptor.rejected);
if (typeof interceptor.runWhen === 'function' && interceptor.runWhen(config) === false) {
return;
}
Copy link
Collaborator

@chinesedfan chinesedfan Mar 6, 2020

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.

Loading

Copy link
Contributor Author

@SashaKoro SashaKoro Apr 11, 2020

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.

Loading

try {
newConfig = onFulfilled(newConfig);
} catch (error) {
onRejected(error);
Copy link
Collaborator

@chinesedfan chinesedfan Mar 6, 2020

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.

Loading

Copy link
Contributor Author

@SashaKoro SashaKoro Apr 11, 2020

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.

Loading

Copy link
Collaborator

@chinesedfan chinesedfan Mar 1, 2021

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.

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Mar 20, 2020

@chinesedfan Hey! I will try to address the requested changes as soon as I can. Been pretty busy.

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Apr 11, 2020

@chinesedfan Hey, just replied to your comments. Sorry for the long delay. Things are pretty hectic.

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented May 25, 2020

@chinesedfan Checking in on the status of this merge request.

Loading

@BruceHubbard
Copy link

@BruceHubbard BruceHubbard commented Jun 19, 2020

@chinesedfan Did you see @SashaKoro's comments? We're still running on a fork of this library for now until this MR gets merged in.

Loading

@briganti
Copy link

@briganti briganti commented Jan 10, 2021

@chinesedfan @yasuf any plan on merging this PR?

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Feb 23, 2021

@chinesedfan @yasuf @jasonsaayman

It's been a while and this cropped back up on my radar again.

Look, I know this is a big change but its also a valuable change. This merge request is already up to 75 comments and I've come back around and updated things several times. My last comments from 10 months did not receive a response. What is preventing this merge request from getting merged? (Besides getting the branch up to date)

Loading

@jasonsaayman
Copy link
Member

@jasonsaayman jasonsaayman commented Feb 23, 2021

@SashaKoro I will look at this in the evening my time and see if we can merge it. Can you tell me if this is up to date with the latest Axios version? Also, are we breaking any backward compatibility here? I have updated the branch so let's see if that comes out successful.

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Feb 23, 2021

@jasonsaayman

Hey Jay. Thanks for the quick reply. I'm gonna get myself up to speed with the latest axios version + backward compatibility this evening / tomorrow and get back to you.

Loading

@SashaKoro
Copy link
Contributor Author

@SashaKoro SashaKoro commented Feb 25, 2021

@jasonsaayman

I've done a thorough look through the axios commits to master since the original merge request timeline and did not find any potential issues. The merge request should break no backward capabilities.

Loading

@jasonsaayman jasonsaayman self-assigned this Mar 1, 2021
@jasonsaayman jasonsaayman added this to the v0.22.0 milestone Mar 1, 2021
@jasonsaayman jasonsaayman merged commit 62d6256 into axios:master Mar 1, 2021
1 check passed
Loading
@jasonsaayman jasonsaayman added this to Done in v0.22.0 Mar 17, 2021
jeremyxu2010 pushed a commit to jeremyxu2010/axios that referenced this issue Jul 2, 2021
Glandos added a commit to Glandos/vue-auth-plugin that referenced this issue Sep 8, 2021
Since axios/axios#2702 (in axios 0.21.2) it is possible to avoid delay in requests.
Interceptors set up by vue-auth are global, and are delaying every other callers.
josephzidell added a commit to josephzidell/axios that referenced this issue Sep 8, 2021
PR axios#2702 adds functionality to define interceptor behavior, but it doesn't add type definitions.
josephzidell added a commit to josephzidell/axios that referenced this issue Sep 8, 2021
jeffberry added a commit to jeffberry/axios that referenced this issue Oct 16, 2021
Updates Typescript definition file to include types for the interceptor options that were added in axios#2702 so that Typescript users are able to safely consume this feature.
yepitschunked added a commit to yepitschunked/axios-retry that referenced this issue Nov 4, 2021
Axios recently added an option to specify whether a request interceptor is synchronous. See axios/axios#2702.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
v0.22.0
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants