Add ability to merge instances #510

szmarczak · 2018-07-09T15:52:00Z

WHY?

Imagine there are got plugins. You can limit download & upload, use GitHub API, send random User Agent and many more. The possibilities are infinite. What if you want to merge some of them into one single piece? Well, got.merge() comes with help.

const singleInstance = got.merge([limitInstance, ghInstance, randomUAInstance, ...]);

DONE

It's working!
More tests
Update advanced-creation.md
Stores only handlers of merged instances instead of whole instances
Split this PR into smaller pieces: Expose assignOptions #530 & Pass normalized options to the handler #532
Provide some useful use-cases

TODO

Discuss possible alternatives

DROPPED TASKS

Removing instances from a chain
Checking if a chain has chosen instance
Getting an array of used instances: got.instances.

Why these tasks were dropped?

That's because defaults are merged, so it's another single instance now. The instances you have provided aren't stored.

brandon93s · 2018-07-09T16:55:08Z

What is the use case for this?

E.g. one instance can limit download & upload and second instance may set some headers. Join them through the got.forward function! instanceC = instanceA.forward(instanceB)

Does this imply that an app would be using three instances of got? Seems that this could just be handled by constructing / merging numerous config objects together instead of mashing instances together.

sindresorhus · 2018-07-09T17:28:36Z

test/create.js

@@ -99,7 +99,7 @@ test('custom endpoint with custom headers (extend)', async t => {
 		json: true
 	})).body;
 	t.is(headers.unicorn, 'rainbow');
-	t.is(headers['user-agent'] === undefined, false);
+	t.false(headers['user-agent'] === undefined);


t.not(headers['user-agent'], undefined);

Same with the other test.

sindresorhus · 2018-07-09T17:30:56Z

I'm also struggling to imagine a use-case for this. What actual (not just example) problem does this solve? I'm not saying it's not a good feature, I just don't see when I would use it.

sindresorhus · 2018-07-09T17:33:18Z

advanced-creation.md

@@ -114,3 +112,31 @@ const unicorn = got.create(settings);
 // Same as:
 const unicorn = got.extend({headers: {unicorn: 'rainbow'}});
 ```
+
+#### got.forward(instance)


I think forward is a confusing name as it sounds like you're forwarding the request. If this were to be added, it would make more sense to just let got.extend and got.create accept a parent instance.

What if you use two different libs (instances of got)? Then you have no possibility to do that. Wait, you could do that this way: got.extend(instance)

got.extend(instance) people would think it'll only merge options (not handlers), the same as got.extend(instance.defaults.options) but no the same as got.forward(instance). I'll rename it to got.merge.

szmarczak · 2018-07-09T18:01:16Z

@brandon93s You're right, I should've documented that better.

Does this imply that an app would be using three instances of got?

No. But it says so, I need to change the docs then.

Seems that this could just be handled by constructing / merging numerous config objects together instead of mashing instances together.

That's how it's done!

https://github.com/szmarczak/got/blob/074462ae98c8ea27108db17d8d59ee10199bd3c3/source/create.js#L46-L50

Note: this comment is outdated.

szmarczak · 2018-07-09T18:08:53Z

I used to do limit(gotInstance, bytes) to limit download & upload or
process(() => limit(gotInstance, bytes)) when I needed to make request many times + limit that.

Instead of process(() => limit(gotInstance, bytes)) we can do something like

const instance = process.merge(limit);

instance(url, { limit: bytes })

szmarczak · 2018-07-09T19:27:38Z

Real example (note: not tested): https://gist.github.com/szmarczak/86217216ad2fbc0c8a3e833e5f290460

sindresorhus · 2018-07-10T11:35:18Z

@szmarczak You got a comment on your gist: https://gist.github.com/szmarczak/86217216ad2fbc0c8a3e833e5f290460#gistcomment-2642454 (Mentioning since there are no notifications for gists).

sindresorhus · 2018-07-10T11:37:22Z

@brandon93s Can you do your gist comment here instead? As it's easier to follow the discussion here.

szmarczak · 2018-07-10T11:43:50Z

@brandon93s Yeah. We could do something like this:

const merge = (a, b) => got.create({
	methods: a.defaults.methods,
	options: got.assignOptions(a.defaults.options, b.defaults.options),
    handler: (options, next) => a.defaults.handler(options, options => b.defaults.handler(options, next));
});

const ghLimit = merge(ghGot, limit);

But consider merging many instances. Think about plugins, like limiting download & upload, switching to HTTPS where possible, new options etc. :)

szmarczak · 2018-07-13T16:16:33Z

advanced-creation.md

@@ -11,7 +11,8 @@ Configure a new `got` instance with the provided settings.<br>

 ##### [options](readme.md#options)

-To inherit from parent, set it as `got.defaults.options` or use [object spread](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#Spread_in_object_literals).
+To inherit from parent, set it as `got.defaults.options` or use `got.assignOptions(defaults.options, options)`.<br>
+Avoid using [object spread](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax#Spread_in_object_literals) as it may give unwanted result.


const a = {headers: {cat: 'meow'}}; const b = {headers: {dog: 'woof'}}; {...a, ...b} // => {headers: {dog: 'woof'}} got.assignOptions(a, b) // => {headers: {cat: 'meow', dog: 'woof'}}

szmarczak · 2018-07-16T20:06:50Z

@jstewmon Can you review this? I'd be very grateful if you left some notes 😃

brandon93s

I'm still not convinced this is necessary. It adds complexity (maintenance risk) to the code base for a very advanced used case. Since extensive documentation is already needed for it, we could offer tips on how to merge instances in the readme as opposed to offering it in core. A separate module could also support this need.

brandon93s · 2018-07-17T21:56:59Z

source/as-stream.js

@@ -14,6 +14,8 @@ module.exports = options => {

 	options.gotRetry.retries = () => 0;

+	options.gotRetry.retries = () => 0;


Isn't this the same as line 15?

Yes, I forgot to remove it when I was resolving a merge conflict.

brandon93s · 2018-07-17T21:58:34Z

advanced-creation.md

+
+#### got.merge(instances, [methods])
+
+Merges many instances into a single one.


Would suggest we document the merge strategy. Which options are merged, in what order, etc.

szmarczak · 2018-07-18T10:00:56Z

It adds complexity (maintenance risk) to the code base for a very advanced used case.

Maybe.

Since extensive documentation is already needed for it, we could offer tips on how to merge instances in the readme as opposed to offering it in core.

👍

A separate module could also support this need.

A separate file? Of course, but it's very hard to do so. create requires merge, merge requires create and so on. I know Node.JS supports circular require() calls, but I don't think it's gonna work.

Another idea:

got.register('functionName', function() {});
got.functionName();

sindresorhus · 2018-07-18T17:28:29Z

There might be use-cases for this, but I'm a little bit reluctant as this locks us into supporting the ability to arbitrarily merge instances. There might be features we want to add in the future that makes this difficult. Like @brandon93s said, it also adds some maintenance overhead. And we have yet to figure out a way to support plugins, which might or might not overlap or conflict with this.

So in short, I just feel like this is rushing into supporting a huge API surface without enough time and exploration to see what problems it solves and the alternatives.

sindresorhus · 2018-07-18T17:30:28Z

Imagine there are got plugins. You can limit download & upload, use GitHub API, send random User Agent and many more.

We should consider what other ways there are to solve these problems.

szmarczak · 2018-07-18T19:05:45Z

There might be use-cases for this, but I'm a little bit reluctant as this locks us into support the ability to arbitrarily merge instances.

Indeed.

There might be features we want to add in the future that makes this difficult.

That's with every single feature, isn't it? :)

And we have yet to figure out a way to support plugins, which might or might not overlap or conflict with this.

So in short, I just feel like this is rushing into supporting a huge API surface without enough time and exploration to see what problems it solves and the alternatives.

Understood. Better to consider all the alternatives we have here. I'll leave this PR open. This feature is too complicated to release it now.

Tomorrow I'll send two PRs with these changes:

expose assignOptions to enable proper options merging (custom instances)
better handler function: (normalizedOptions, next) instead of (options, next)
- move baseUrl option from main handler to normalize-arguments.js

sindresorhus · 2018-08-09T13:12:51Z

advanced-creation.md

+
+##### [[methods]](#methods)
+
+Default: `instances[0].defaults.methods`


What's the use-case for this?

It's not needed at all (that was an old idea). These are just an aliases to the instance. (got.get(), got.post() etc) Hang on, I'll fix it.

…ot-forward

szmarczak · 2018-08-09T14:14:34Z

advanced-creation.md

+
+##### [instances](readme.md#instances)
+
+## Usage


Should the examples be moved to the top?

sindresorhus · 2018-08-09T19:19:01Z

test/create.js

@@ -102,7 +102,6 @@ test('extend merges URL instances', t => {
 test('create', async t => {
 	const instance = got.create({
 		options: {},
-		methods: ['get'],


Why are you suddenly removing all the methods fields?

These are just aliases. If methods are set to ['get'] you can't do got.post() but you can do got(url, {method: 'POST'});. I see no reason why we should let users to modify the aliases.

I agree 👍

sindresorhus · 2018-08-09T19:19:30Z

source/normalize-arguments.js

@@ -11,7 +11,7 @@ const urlToOptions = require('./url-to-options');
 const isFormData = require('./is-form-data');

 const retryAfterStatusCodes = new Set([413, 429, 503]);
-const knownHookEvents = ['beforeRequest'];
+const knownHookEvents = require('./known-hook-events');


require statements should be at the top of the file. In this case, line 12.

sindresorhus · 2018-08-09T19:28:13Z

advanced-creation.md

+- options are merged using [`got.mergeOptions()`](readme.md#gotmergeoptionsparentoptions-newoptions) (+ hooks are merged too),
+- handlers are stored in an array.
+
+## Usage


sindresorhus · 2018-08-09T19:29:29Z

advanced-creation.md

@@ -111,3 +112,119 @@ const unicorn = got.create(settings);
 // Same as:
 const unicorn = got.extend({headers: {unicorn: 'rainbow'}});
 ```
+
+### Merging instances


Can you add an entry to the Highlights section in the readme called Composable, which links to this section?

sindresorhus · 2018-08-09T19:31:33Z

advanced-creation.md

@@ -111,3 +112,119 @@ const unicorn = got.create(settings);
 // Same as:
 const unicorn = got.extend({headers: {unicorn: 'rainbow'}});
 ```
+
+### Merging instances
+


I think it would be nice to have something like this as an intro. Feel free to improve it.

Got supports composing multiple instances together. This is very powerful. You can create a client that limits download speed and then compose it with an instance that signs a request. It's like plugins without any of the plugin mess. You just create instances and then compose them together.

Sounds good!

sindresorhus · 2018-08-09T19:32:11Z

advanced-creation.md

+- handlers are stored in an array.
+
+## Usage
+


Some examples of what kind of instances you could compose together.

sindresorhus · 2018-08-09T19:32:54Z

advanced-creation.md

+});
+```
+
+#### Limiting download & upload (in case your machine's got a little amount of RAM)


(in case your machine's got a little amount of RAM) should be in text below the title.

sindresorhus · 2018-08-09T19:34:11Z

advanced-creation.md

+
+If these ^^^ are different modules and you don't want to rewrite them, use `got.mergeInstances()`.
+
+**Note**: `noUserAgent` must be placed at the end of chain as the instances are merged serially. Other modules do have the `user-agent` header.


noUserAgent => The noUserAgent instance

are merged serially => are merged in order

Other modules => Other instances

sindresorhus · 2018-08-09T19:35:48Z

advanced-creation.md

+```
+
+If these ^^^ are different modules and you don't want to rewrite them, use `got.mergeInstances()`.
+


There should be a header here called #### Putting it all together or something.

sindresorhus · 2018-08-10T07:13:35Z

This is looking good now :)

sindresorhus requested changes Jul 9, 2018

View reviewed changes

sindresorhus reviewed Jul 9, 2018

View reviewed changes

szmarczak changed the title ~~Introduce got.forward(options)~~ Introduce got.merge(options) Jul 9, 2018

szmarczak closed this Jul 10, 2018

szmarczak reopened this Jul 10, 2018

szmarczak changed the title ~~Introduce got.merge(options)~~ Merging instances Jul 10, 2018

sindresorhus requested a review from alextes July 11, 2018 12:00

szmarczak mentioned this pull request Jul 11, 2018

Add beforeRequest hook #516

Merged

szmarczak force-pushed the got-forward branch from 08e14f5 to 6ab368e Compare July 13, 2018 16:00

szmarczak requested review from sindresorhus and brandon93s July 13, 2018 16:12

szmarczak commented Jul 13, 2018

View reviewed changes

szmarczak mentioned this pull request Jul 14, 2018

Fix: formatted query should be assigned to options.search #519

Merged

szmarczak force-pushed the got-forward branch 3 times, most recently from e389280 to 39982d8 Compare July 17, 2018 20:00

brandon93s reviewed Jul 17, 2018

View reviewed changes

sindresorhus reviewed Aug 9, 2018

View reviewed changes

szmarczak and others added 10 commits August 9, 2018 15:33

Update create.js

694d613

Update readme.md

a4fcb66

Remove defaults.methods

c5fb871

Merge branch 'master' into got-forward

e7802cf

Style tweaks

0e5f6f6

Merge branch 'got-forward' of https://github.com/szmarczak/got into g…

8515525

…ot-forward

Improve docs

e242619

Docs tweaks

8a59d8a

typo

179e682

Fix docs

b188b52

szmarczak commented Aug 9, 2018

View reviewed changes

advanced-creation.md Outdated

##### [instances](readme.md#instances)

## Usage

Copy link

Collaborator Author

szmarczak Aug 9, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the examples be moved to the top?

szmarczak requested a review from sindresorhus August 9, 2018 14:19

sindresorhus reviewed Aug 9, 2018

View reviewed changes

Update readme.md

b73a804

sindresorhus reviewed Aug 9, 2018

View reviewed changes

Improve advanced-creation.md

c5e812a

szmarczak requested a review from sindresorhus August 10, 2018 06:55

sindresorhus merged commit f447378 into sindresorhus:master Aug 10, 2018

sindresorhus pushed a commit that referenced this pull request Aug 10, 2018

Add ability to merge instances (#510)

19a62e8

sindresorhus pushed a commit that referenced this pull request Aug 10, 2018

Add ability to merge instances (#510)

f0b190a

szmarczak deleted the got-forward branch January 17, 2019 18:55

		@@ -14,6 +14,8 @@ module.exports = options => {

		options.gotRetry.retries = () => 0;

		options.gotRetry.retries = () => 0;


		#### got.merge(instances, [methods])

		Merges many instances into a single one.


		##### [[methods]](#methods)

		Default: `instances[0].defaults.methods`


		If these ^^^ are different modules and you don't want to rewrite them, use `got.mergeInstances()`.

		Note: `noUserAgent` must be placed at the end of chain as the instances are merged serially. Other modules do have the `user-agent` header.

Add ability to merge instances #510

Add ability to merge instances #510

Conversation

szmarczak commented Jul 9, 2018 • edited

WHY?

DONE

TODO

DROPPED TASKS

Why these tasks were dropped?

brandon93s commented Jul 9, 2018

Choose a reason for hiding this comment

sindresorhus commented Jul 9, 2018

Choose a reason for hiding this comment

szmarczak Jul 9, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

szmarczak commented Jul 9, 2018 • edited

szmarczak commented Jul 9, 2018 • edited

szmarczak commented Jul 9, 2018 • edited

sindresorhus commented Jul 10, 2018

sindresorhus commented Jul 10, 2018

szmarczak commented Jul 10, 2018 • edited

szmarczak Jul 13, 2018 • edited

Choose a reason for hiding this comment

szmarczak commented Jul 16, 2018

brandon93s 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

Choose a reason for hiding this comment

szmarczak commented Jul 18, 2018 • edited

sindresorhus commented Jul 18, 2018 • edited

sindresorhus commented Jul 18, 2018

szmarczak commented Jul 18, 2018 • edited

Choose a reason for hiding this comment

szmarczak Aug 9, 2018 • edited

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

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

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

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

Choose a reason for hiding this comment

sindresorhus commented Aug 10, 2018

szmarczak commented Jul 9, 2018 •

edited

szmarczak Jul 9, 2018 •

edited

szmarczak commented Jul 9, 2018 •

edited

szmarczak commented Jul 9, 2018 •

edited

szmarczak commented Jul 9, 2018 •

edited

szmarczak commented Jul 10, 2018 •

edited

szmarczak Jul 13, 2018 •

edited

szmarczak commented Jul 18, 2018 •

edited

sindresorhus commented Jul 18, 2018 •

edited

szmarczak commented Jul 18, 2018 •

edited

szmarczak Aug 9, 2018 •

edited