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

Fixed The timeoutErrorMessage property in config not work with Node.js (fixes #3580) #3581

Conversation

duibu05
Copy link
Contributor

@duibu05 duibu05 commented Jan 22, 2021

This PR fixes #3580, the issue is that when send axios request in Node.js contains timeout, timeoutErrorMessage properties in config then fired timeout, the error message is not the customized timeoutErrorMessage but "timeout of *ms exceeded". This PR fixes that problem.

duibu05 added 4 commits Jan 22, 2021
…os#3580)

* Adding "should respect the timeoutErrorMessage property" test case

Co-authored-by: Will Loo <duibu05@126.com>
…os#3580)

* Fixing The timeoutErrorMessage property in config not work with Node.js (axios#3580)

* Updating http adapter

* Adding reject config.timeoutErrorMessage when setup

Co-authored-by: Will Loo <duibu05@126.com>
Copy link
Contributor

@djs113 djs113 left a comment

Isn't is better to declare var timeoutErrorMessage = '' first then check if config has a timeoutErrorMessage property and if so assign it this value, else the default time out error message should be displayed. This will ensure that the default message is not unnecessarily created if config already has the error message.

var timeoutErrorMessage = '';
if (config.timeoutErrorMessage) {
  timeoutErrorMessage = config.timeoutErrorMessage;
} else {
  timeoutErrorMessage = 'timeout of ' + timeout + 'ms exceeded';
}

duibu05 added 2 commits Aug 17, 2021
…js (axios#3580)

* Fixing The timeoutErrorMessage property in config not work with Node.js (axios#3580)

* Update http adapter

* Make changes as suggested after code review

Co-authored-by: Will Loo <duibu05@126.com>
@duibu05
Copy link
Contributor Author

duibu05 commented Aug 17, 2021

Isn't is better to declare var timeoutErrorMessage = '' first then check if config has a timeoutErrorMessage property and if so assign it this value, else the default time out error message should be displayed. This will ensure that the default message is not unnecessarily created if config already has the error message.

var timeoutErrorMessage = '';
if (config.timeoutErrorMessage) {
  timeoutErrorMessage = config.timeoutErrorMessage;
} else {
  timeoutErrorMessage = 'timeout of ' + timeout + 'ms exceeded';
}

@daniel-julius-sam done

@djs113
Copy link
Contributor

djs113 commented Aug 18, 2021

Please link this PR to the relevant issue by including fixes #3580 in the PR description.

@duibu05 duibu05 changed the title Fixing The timeoutErrorMessage property in config not work with Node.js (#3580) Fixes #3580 The timeoutErrorMessage property in config not work with Node.js (fixes #3580) Aug 19, 2021
@duibu05 duibu05 changed the title Fixes #3580 The timeoutErrorMessage property in config not work with Node.js (fixes #3580) Fixed The timeoutErrorMessage property in config not work with Node.js (fixes #3580) Aug 19, 2021
@djs113
Copy link
Contributor

djs113 commented Aug 19, 2021

That's not what I meant, you should add fixes #3580 exactly in the main message describing this PR just beneath the title, so that it gets linked to the issue it solves. Like how I have done in this PR. https://github.com/axios/axios/pull/3961#partial-pull-merging

@duibu05
Copy link
Contributor Author

duibu05 commented Aug 19, 2021

@djs113 Got it, already updated. Please approve this workflow.

@djs113
Copy link
Contributor

djs113 commented Aug 19, 2021

I'm not a mainainer, @jasonsaayman please approve this.

@jasonsaayman
Copy link
Member

jasonsaayman commented Dec 22, 2021

@duibu05 please can you mend the conflict?

@jasonsaayman jasonsaayman added the follow-up Issue or pull request requires a follow. label Dec 22, 2021
@duibu05
Copy link
Contributor Author

duibu05 commented Dec 27, 2021

@duibu05 please can you mend the conflict?

@jasonsaayman done

@jasonsaayman jasonsaayman merged commit 4461761 into axios:master Jan 18, 2022
3 checks passed
mbargiel pushed a commit to mbargiel/axios that referenced this issue Jan 27, 2022
fixes axios#3580) (axios#3581)

* The timeoutErrorMessage property in config not work with Node.js (axios#3580)

* Adding "should respect the timeoutErrorMessage property" test case

Co-authored-by: Will Loo <duibu05@126.com>

* The timeoutErrorMessage property in config not work with Node.js (axios#3580)

* Fixing The timeoutErrorMessage property in config not work with Node.js (axios#3580)

* Updating http adapter

* Adding reject config.timeoutErrorMessage when setup

Co-authored-by: Will Loo <duibu05@126.com>

* Fixing The timeoutErrorMessage property in config not work with Node.js (axios#3580)

* Fixing The timeoutErrorMessage property in config not work with Node.js (axios#3580)

* Update http adapter

* Make changes as suggested after code review

Co-authored-by: Will Loo <duibu05@126.com>

Co-authored-by: Jay <jasonsaayman@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
follow-up Issue or pull request requires a follow.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants