Fix reusing Got options #1127

mpern · 2020-03-21T12:07:03Z

Restores the original options.url after calling the init hooks.

Fixes #1118

Checklist

I have read the documentation.
I have included a pull request description of my changes.
I have included some tests.
If it's a new feature, I have included documentation updates.

sindresorhus · 2020-03-21T12:24:08Z

Can you give the PR a more descriptive title?

szmarczak · 2020-03-21T12:44:41Z

test/hooks.ts

@@ -193,7 +193,7 @@ test('init is called with options', withServer, async (t, server, got) => {
 			init: [
 				options => {
 					t.is(options.url, undefined);
-					t.is(options.context, context);
+					t.deepEqual(options.context, context);


szmarczak · 2020-03-21T12:44:46Z

test/hooks.ts

@@ -211,7 +211,7 @@ test('init from defaults is called with options', withServer, async (t, server,
 			init: [
 				options => {
 					t.is(options.url, undefined);
-					t.is(options.context, context);
+					t.deepEqual(options.context, context);


szmarczak · 2020-03-21T12:44:59Z

test/post.ts

@@ -264,7 +264,7 @@ test('the `form` payload is not touched', withServer, async (t, server, got) =>
 		hooks: {
 			beforeRequest: [
 				options => {
-					t.is(options.form, object);
+					t.deepEqual(options.form, object);


szmarczak

Why so many changes? Just move

got/source/normalize-arguments.ts

Lines 313 to 317 in 62b3388

    
           if (hasUrl) { 
        
           	options = mergeOptions(defaults?.options ?? {}, options ?? {}); 
        
           } else { 
        
           	options = mergeOptions(defaults?.options ?? {}, url as object, options ?? {}); 
        
           }

before that if.

mpern · 2020-03-21T14:35:27Z

source/normalize-arguments.ts

-	} else {
-		runInitHooks(defaults?.options.hooks.init, url as Options);
-		runInitHooks((url as Options).hooks?.init, url as Options);
-
-		if (options) {
-			runInitHooks(defaults?.options.hooks.init, options);
-			runInitHooks(options.hooks?.init, options);
-		}
-	}


I'm not 100% sure why runInitHooks was called twice once for the url as Options object and once for the options parameter.

Since I merge both objects now anyway if necessary (see line 297), I don't think that split is necessary anymore

szmarczak · 2020-03-21T15:23:33Z

Actually my thought was a bad solution too, I think I fixed it. Pushed to this PR. It breaks on frozen options though, but that's fixed in #1051

szmarczak · 2020-03-21T15:24:17Z

There are two runInitHooks: one for the defaults, one for the options. It's called twice because the options aren't merged yet.

mpern · 2020-03-21T15:30:19Z

I would prefer to keep the passed objects immutable, but since you're the maintainer, it's up to you.
Maybe something for the rewrite?

szmarczak · 2020-03-21T15:50:30Z

I would prefer to keep the passed objects immutable

Me too.

since you're the maintainer, it's up to you.

No, I just give some ideas and execute them. The maintainer is @sindresorhus :)

Maybe something for the rewrite?

What do you mean? The rewrite fixes both of these issues.

szmarczak · 2020-03-21T15:54:03Z

I don't know why Travis doesn't see my commits inside this PR...

@mpern Can you run the tests locally and confirm if my changes fix the issue? Feel free to experiment around that.

mpern · 2020-03-21T17:09:25Z

Your fix unfortunately breaks a lot of other tests.

And FYI, running npm test locally doesn't work, xo fails:

  source/index.ts:undefined:undefined
  ✖    0:0   Parsing error: Cannot read property name of undefined

I have no clue why (EDIT: typescript-eslint/typescript-eslint/issues/1746)

For now I use npm run build && npx ava

szmarczak · 2020-03-21T18:30:52Z

And FYI, running npm test locally doesn't work, xo fails:

rm -rf node_modules
npm install

szmarczak · 2020-03-21T18:32:36Z

I don't know why the tests fail either. I'll look more into this tomorrow.

sindresorhus · 2020-03-22T07:42:25Z

test/normalize-arguments.ts

+test('should be able to reuse options', t => {
+	const options: Options = {};
+	normalizeArguments('http://localhost', options);
+	t.notThrows(() => normalizeArguments('http://localhost', options));


Suggested change

t.notThrows(() => normalizeArguments('http://localhost', options));

t.notThrows(() => {

normalizeArguments('http://localhost', options);

});

jaulz · 2020-03-24T17:36:55Z

By the way, this also influences the behaviour of the paginate mechanism since we also reuse Got options there.

szmarczak · 2020-03-24T17:52:39Z

By the way, this also influences the behaviour of the paginate mechanism since we also reuse Got options there.

Generally the whole Got project since it sets options.url whenever you provide an init hook.

szmarczak · 2020-04-12T11:40:10Z

Fixed in #1051

mpern changed the title ~~Bugfix/1118~~ Fix #1118 and #1050 Mar 21, 2020

szmarczak reviewed Mar 21, 2020

View reviewed changes

szmarczak requested changes Mar 21, 2020

View reviewed changes

mpern changed the title ~~Fix #1118 and #1050~~ Use a clone of the options parameter instead of modifiying the passed object directly Mar 21, 2020

mpern commented Mar 21, 2020

View reviewed changes

Markus Perndorfer and others added 3 commits March 21, 2020 15:37

clone 'options' when calling got

4345ac1

Update normalize-arguments.ts

11a99f9

Update normalize-arguments.ts

fbcedd0

szmarczak changed the title ~~Use a clone of the options parameter instead of modifiying the passed object directly~~ Fix reusing Got options Mar 21, 2020

Markus Perndorfer added 2 commits March 21, 2020 17:32

trigger travis

30ac055

fix fomrating

58c4c2c

szmarczak added 5 commits March 21, 2020 19:04

Update normalize-arguments.ts

2f4c74d

Update normalize-arguments.ts

4b4f8ba

Update normalize-arguments.ts

4411e48

Update normalize-arguments.ts

64025b8

Update normalize-arguments.ts

601e453

sindresorhus reviewed Mar 22, 2020

View reviewed changes

szmarczak closed this Apr 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix reusing Got options #1127

Fix reusing Got options #1127

mpern commented Mar 21, 2020 •

edited by szmarczak

Loading

sindresorhus commented Mar 21, 2020

szmarczak Mar 21, 2020

szmarczak Mar 21, 2020

szmarczak Mar 21, 2020

szmarczak left a comment

mpern Mar 21, 2020

szmarczak commented Mar 21, 2020 •

edited

Loading

szmarczak commented Mar 21, 2020 •

edited

Loading

mpern commented Mar 21, 2020

szmarczak commented Mar 21, 2020

szmarczak commented Mar 21, 2020

mpern commented Mar 21, 2020 •

edited

Loading

szmarczak commented Mar 21, 2020 •

edited

Loading

szmarczak commented Mar 21, 2020

sindresorhus Mar 22, 2020

jaulz commented Mar 24, 2020

szmarczak commented Mar 24, 2020

szmarczak commented Apr 12, 2020

	if (hasUrl) {
	options = mergeOptions(defaults?.options ?? {}, options ?? {});
	} else {
	options = mergeOptions(defaults?.options ?? {}, url as object, options ?? {});
	}

Fix reusing Got options #1127

Fix reusing Got options #1127

Conversation

mpern commented Mar 21, 2020 • edited by szmarczak Loading

Checklist

sindresorhus commented Mar 21, 2020

szmarczak Mar 21, 2020

Choose a reason for hiding this comment

szmarczak Mar 21, 2020

Choose a reason for hiding this comment

szmarczak Mar 21, 2020

Choose a reason for hiding this comment

szmarczak left a comment

Choose a reason for hiding this comment

mpern Mar 21, 2020

Choose a reason for hiding this comment

szmarczak commented Mar 21, 2020 • edited Loading

szmarczak commented Mar 21, 2020 • edited Loading

mpern commented Mar 21, 2020

szmarczak commented Mar 21, 2020

szmarczak commented Mar 21, 2020

mpern commented Mar 21, 2020 • edited Loading

szmarczak commented Mar 21, 2020 • edited Loading

szmarczak commented Mar 21, 2020

sindresorhus Mar 22, 2020

Choose a reason for hiding this comment

jaulz commented Mar 24, 2020

szmarczak commented Mar 24, 2020

szmarczak commented Apr 12, 2020

mpern commented Mar 21, 2020 •

edited by szmarczak

Loading

szmarczak commented Mar 21, 2020 •

edited

Loading

szmarczak commented Mar 21, 2020 •

edited

Loading

mpern commented Mar 21, 2020 •

edited

Loading

szmarczak commented Mar 21, 2020 •

edited

Loading