handle developer errors from callbacks separate from fetch errors

[alexkli]

Currently we share the same catch clause for fetch() and for the shouldRetry() here:

node-fetch-retry/index.js

Lines 217 to 227 in 3e512e5

	try {
	const response = await fetch(url, options);
	if (await shouldRetry(retryOptions, null, response, waitTime)) {
	console.error(`Retrying in ${waitTime} milliseconds, attempt ${attempt} failed (status ${response.status}): ${response.statusText}`);
	} else {
	// response.timeout should reflect the actual timeout
	response.timeout = retryOptions.socketTimeout;
	return resolve(response);
	}
	} catch (error) {

shouldRetry includes calling the callbacks retryOnHttpError() and retryOnHttpResponse(). These callbacks could have developer errors on which we should not retry blindly.

Instead, we should separate the error handling, and for errors on the callbacks pass them through as clear errors (and do not retry) so the developers can fix their code.

[tmathern]

+1

[jdelbick]

note: we pass in null for error here: shouldRetry(retryOptions, null, response, waitTime))
so retryOnHttpError will never get called in that case:

node-fetch-retry/index.js

Lines 48 to 51 in 3e512e5

	} else if (retryOptions && retryOptions.retryOnHttpError && error != null) {
	// retryOnHttpError can be sync or async because either the promise or result will be
	// bubbled up to what shouldRetry returns
	return retryOptions.retryOnHttpError(error);

However, I agree we should separate these two functions so its more readable

[alexkli]

@jdelbick but retryOptions.retryOnHttpResponse() will be called

[jdelbick]

@alexkli so you mean the situation if shouldRetry(retryOptions, null, response, waitTime) throws due to a developer error, then it goes to the next try/catch and could retry? makes sense, something like this:

try { 
     const response = await fetch(url, options); 
     try {
        if (await shouldRetry(retryOptions, null, response, waitTime)) { 
            console.error(`Retrying in ${waitTime} milliseconds, attempt ${attempt} failed (status ${response.status}): ${response.statusText}`); 
        } else { 
           // response.timeout should reflect the actual timeout 
           response.timeout = retryOptions.socketTimeout; 
           return resolve(response); 
       } 
     } catch (e) {
        // developer error
     }
 } catch (error) { 

edit, splitting out the try/catches & adding error handling, I get it

try { 
     const response = await fetch(url, options); 
} catch (e) {
  try {
     if (await shouldRetry(retryOptions, e, null, waitTime)) { ...
     }
  } catch (e) { //dev error}
}
try {
        if (await shouldRetry(retryOptions, null, response, waitTime)) { 
            console.error(`Retrying in ${waitTime} milliseconds, attempt ${attempt} failed (status ${response.status}): ${response.statusText}`); 
        } else { 
           // response.timeout should reflect the actual timeout 
           response.timeout = retryOptions.socketTimeout; 
           return resolve(response); 
       } 
 } catch (e) {
        // developer error
 }