Fix default `retry` option value when specifying a number #809

zero1five · 2019-05-26T05:44:15Z

Fixes: #806

Checklist

I have read the documentation.

zero1five · 2019-05-26T05:54:08Z

test('works when defaults.options.retry is not an object', withServer, async (t, server, got) => {
	server.get('/', handler413);

	const instance = got.extend({
		retry: 2
	});

	const {retryCount} = await instance({
		throwHttpErrors: false
	});
	t.is(retryCount, 0);
});

Failed test case made me feel a little confused. 🙇‍♂️

szmarczak

This doesn't work for custom instances

zero1five · 2019-05-26T07:40:16Z

I'll check back later, I think I'm missing something very important.

zero1five · 2019-05-27T05:17:23Z

@szmarczak hi, I studied the test carefully, Before this pr, retryCount didn't seem to be running properly.

got/test/retry.ts

Lines 277 to 289 in f77db99

    
           test('works when defaults.options.retry is not an object', withServer, async (t, server, got) => { 
        
           	server.get('/', handler413); 
        
           	const instance = got.extend({ 
        
           		retry: 2 
        
           	}); 
        
           	const {retryCount} = await instance({ 
        
           		throwHttpErrors: false 
        
           	}); 
        
           	t.is(retryCount, 0); 
        
           });

new instance created with extend should include the 413 statusCodes (if the default statusCodes is not modified).this use case was previously successful because it did not include 413( empty Set).

Maybe we should remove 413 out of the default status code?

sindresorhus · 2019-06-17T10:07:14Z

Hey, sorry, we just merged #797 which caused a lot of merge conflicts. Would you be able to fix the conflicts?

szmarczak · 2019-06-17T20:32:56Z

source/normalize-arguments.ts

+		}
+	} else {
+		options.retry = {
+			..._innerDefaults.options.retry,


This line (91) seem to be useless.

source/normalize-arguments.ts

source/index.ts

szmarczak · 2019-06-17T20:37:52Z

source/normalize-arguments.ts


 	if (retry !== false) {
+		options.retry = _innerDefaults.options.retry;


Is this line really needed?

Yes, in #806 default configure are missing.

zero1five · 2019-06-18T01:00:45Z

@szmarczak Hi, I have made some modifications, Would you please have a look at it again?

All test cases so far have only one failure, which is here, I'm not sure if there's any historical reason for it. Can you take a look

Thanks!

zero1five · 2019-06-18T01:04:30Z

CI:

  1 test failed
  2 tests skipped
  retry.ts › works when defaults.options.retry is not an object
  /home/travis/build/sindresorhus/got/test/retry.ts:287
   286:   });                 
   287:   t.is(retryCount, 0);
   288: });                   
  Difference:
  - 2
  + 0

sindresorhus · 2019-06-20T06:44:49Z

Yeah, it's unclear to me why it was previously testing against 0. @szmarczak ?

szmarczak · 2019-06-20T12:39:29Z

It was introduced by 10d22b7, before the Retry-After functionality. It was testing against 0 because at that time it wasn't retrying on 413.

Looks like a bug now and it should be tested against 2.

szmarczak · 2019-06-20T12:42:47Z

Ah, I get it. Using the retry option with got.extend has been broken, so it was passing. This PR fixes that. I'll review this PR soon.

szmarczak

This PR doesn't change anything at all. The merge logic is broken, not the preNormalizeArguments. I think it should preNormalize all sources before merging.

szmarczak · 2019-06-20T12:44:09Z

source/index.ts

-	mutableDefaults: false
-};
-
-const got = create(defaults);


Let's leave the defaults here. No need to create a new file.

szmarczak · 2019-06-20T13:11:00Z

source/normalize-arguments.ts

-		statusCodes: new Set(),
-		errorCodes: new Set(),
-		maxRetryAfter: undefined
+		...defaultOptions.options.retry as RetryOption,


This is wrong. It should be defaults.retry so we get to the same point as before.

szmarczak · 2019-06-20T13:43:16Z

source/merge.ts

-			return iterate(options);
-		}
-	};
-}


We need to move this out because if we are going to pre-normalize all options before merging, we need to avoid require collisions (self-requiring files).

szmarczak · 2019-06-20T14:25:19Z

Renaming options.retry to options.retry.retries should fix everything...

sindresorhus · 2019-06-20T17:54:39Z

source/merge.ts

-	const mergedOptions = merge({} as T & {hooks: Partial<Hooks>}, ...sources.map(source => source || {}));
+export function mergeOptions<T extends Options>(...sources: T[]): T & { hooks: Partial<Hooks> } {
+	sources = sources.map(source => {
+		if (source) {


I would invert this an do an early return, to reduce nesting.

sindresorhus · 2019-06-20T17:54:49Z

source/merge.ts

+export function mergeOptions<T extends Options>(...sources: T[]): T & { hooks: Partial<Hooks> } {
+	sources = sources.map(source => {
+		if (source) {
+			if (!is.object(source.retry)) {


sindresorhus · 2019-06-20T17:55:09Z

source/merge.ts

+		return {};
+	}) as T[];
+
+	const mergedOptions = merge({} as T & { hooks: Partial<Hooks> }, ...sources);


Suggested change

const mergedOptions = merge({} as T & { hooks: Partial<Hooks> }, ...sources);

const mergedOptions = merge({} as T & {hooks: Partial<Hooks>}, ...sources);

sindresorhus · 2019-06-20T17:55:20Z

source/merge.ts

@@ -42,8 +42,25 @@ export default function merge<Target extends Record<string, unknown>, Source ext
 	return target as Target & Source;
 }

-export function mergeOptions<T extends Options>(...sources: T[]): T & {hooks: Partial<Hooks>} {
-	const mergedOptions = merge({} as T & {hooks: Partial<Hooks>}, ...sources.map(source => source || {}));
+export function mergeOptions<T extends Options>(...sources: T[]): T & { hooks: Partial<Hooks> } {


Suggested change

export function mergeOptions<T extends Options>(...sources: T[]): T & { hooks: Partial<Hooks> } {

export function mergeOptions<T extends Options>(...sources: T[]): T & {hooks: Partial<Hooks>} {

Co-authored-by: Szymon Marczak <sz.marczak@gmail.com>

…us#809) Co-authored-by: Szymon Marczak <sz.marczak@gmail.com>

szmarczak requested changes May 26, 2019

View reviewed changes

szmarczak reviewed Jun 17, 2019

View reviewed changes

source/normalize-arguments.ts Outdated Show resolved Hide resolved

szmarczak reviewed Jun 17, 2019

View reviewed changes

source/index.ts Outdated Show resolved Hide resolved

szmarczak reviewed Jun 17, 2019

View reviewed changes

zero1five force-pushed the fix/options-retry branch 2 times, most recently from ce54af1 to c568f09 Compare June 18, 2019 00:52

sindresorhus changed the title ~~retry: fix default value when enter a number~~ Fix default retry option value when specifying a number Jun 20, 2019

szmarczak requested changes Jun 20, 2019

View reviewed changes

szmarczak reviewed Jun 20, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

Fix merging retry

2f5da71

szmarczak force-pushed the fix/options-retry branch from 9c71db8 to 2f5da71 Compare June 20, 2019 14:23

Merge branch 'master' into fix/options-retry

06df3b2

Remove trailing empty lines

66c89fb

sindresorhus reviewed Jun 20, 2019

View reviewed changes

nitpicks

d5d6885

sindresorhus merged commit 77e4c49 into sindresorhus:master Jun 21, 2019

sindresorhus pushed a commit that referenced this pull request Jun 21, 2019

Fix default retry option value when specifying a number (#809)

9c04a7c

Co-authored-by: Szymon Marczak <sz.marczak@gmail.com>

szmarczak added a commit to szmarczak/got that referenced this pull request Jun 25, 2019

Fix default retry option value when specifying a number (sindresorh…

d97532d

…us#809) Co-authored-by: Szymon Marczak <sz.marczak@gmail.com>

Fix default `retry` option value when specifying a number #809

Fix default `retry` option value when specifying a number #809

zero1five commented May 26, 2019

zero1five commented May 26, 2019

szmarczak left a comment

zero1five commented May 26, 2019

zero1five commented May 27, 2019 •

edited

sindresorhus commented Jun 17, 2019

szmarczak Jun 17, 2019 •

edited

szmarczak Jun 17, 2019

zero1five Jun 17, 2019

zero1five commented Jun 18, 2019

zero1five commented Jun 18, 2019

sindresorhus commented Jun 20, 2019

szmarczak commented Jun 20, 2019

szmarczak commented Jun 20, 2019

szmarczak left a comment

szmarczak Jun 20, 2019

szmarczak Jun 20, 2019

szmarczak Jun 20, 2019

This comment has been minimized.

This comment has been minimized.

szmarczak commented Jun 20, 2019

sindresorhus Jun 20, 2019

sindresorhus Jun 20, 2019

sindresorhus Jun 20, 2019

sindresorhus Jun 20, 2019


		if (retry !== false) {
		options.retry = _innerDefaults.options.retry;

	const mergedOptions = merge({} as T & { hooks: Partial<Hooks> }, ...sources);
	const mergedOptions = merge({} as T & {hooks: Partial<Hooks>}, ...sources);

	export function mergeOptions<T extends Options>(...sources: T[]): T & { hooks: Partial<Hooks> } {
	export function mergeOptions<T extends Options>(...sources: T[]): T & {hooks: Partial<Hooks>} {

Fix default retry option value when specifying a number #809

Fix default retry option value when specifying a number #809

Conversation

zero1five commented May 26, 2019

Checklist

zero1five commented May 26, 2019

szmarczak left a comment

Choose a reason for hiding this comment

zero1five commented May 26, 2019

zero1five commented May 27, 2019 • edited

sindresorhus commented Jun 17, 2019

szmarczak Jun 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zero1five commented Jun 18, 2019

zero1five commented Jun 18, 2019

sindresorhus commented Jun 20, 2019

szmarczak commented Jun 20, 2019

szmarczak commented Jun 20, 2019

szmarczak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

szmarczak commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fix default `retry` option value when specifying a number #809

Fix default `retry` option value when specifying a number #809

zero1five commented May 27, 2019 •

edited

szmarczak Jun 17, 2019 •

edited