-
-
Notifications
You must be signed in to change notification settings - Fork 737
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
Add an error code to NetConnectNotAllowedError #898
Conversation
As I articulated in #884, NetConnectNotAllowedError is a programmer error, not a runtime error. Nock should treat it less like a network error, not more. I would argue against this change. |
To say this another way:
|
I'm using tools that retry on recoverable errors but not others. ( |
@reconbot can you pass a function to the retry option that checks for the |
It's feels more weird to introduce nock to my production code than it does
to simulate a network error when I want to simulate a network error.
…On Fri, Sep 22, 2017, 8:35 PM Gregor Martynus ***@***.***> wrote:
@reconbot <https://github.com/reconbot> can you pass a function to the
retry option that checks for the NetConnectNotAllowedError error name and
use that as a way to not retry the request?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#898 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABlbloLsAu4EAykGvA3sGfqwMtKXvHAks5slFJugaJpZM4NYgE->
.
|
I don’t understand what you mean Francis :) Is this PR still relevant for you or can we close it? And if it is, can you explain in more detail? |
Say I have an application function import got from 'got'
import {authHeaders} from './config'
export default function (url) {
return got(url, { authHeaders, retries: 5 })
} And I don't properly mock this request in my tests.
I don't want to import I'm proposing we use a standard error code that suggests the requests shouldn't be retried when preventing a network connection. The specific code isn't important to me but I know |
I’m okay merging it. Could you rebase your PR on the latest master? |
This is in line with nodejs error codes. I chose `ENETUNREACH` because it clearly defines the reason why the http request failed. https://nodejs.org/api/all.html#errors_error_code
27112ae
to
088e315
Compare
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you! |
This is in line with nodejs error codes. I chose
ENETUNREACH
because it clearly defines the reason why the http request failed.This helps testing when using an http request library that does retries as this isn't an error you would normally retry.