Simplify wrapping `got` #503

szmarczak · 2018-07-06T13:34:00Z

Fixes #203.

Real example:

https://gist.github.com/szmarczak/6255d5fa9e2f0f0e15ef2264be867478

sindresorhus · 2018-07-06T18:45:40Z

I'm a little bit worried that we're exposing too many internals now, especially got.normalizeArguments.

sindresorhus · 2018-07-06T18:45:57Z

What's the difference between .fork() and .create()?

sindresorhus · 2018-07-06T18:47:12Z

I really like how much it simplifies gh-got though. I'm sure it will enable more useful niche versions of Got.

szmarczak · 2018-07-06T19:01:58Z

What's the difference between .fork() and .create()?

Fork - forks the options from parent instance (squashes them into a new piece, performs assignOptions again), replaces methods and handler if provided.

Create - imagine there are no options, no methods, no handler. You set it up from scratch. Like another got, not inherited from any instance.

sindresorhus · 2018-07-06T19:04:52Z

We should list fork() first then, as that's what most end-users would want. They just want to tweak some of the defaults.

sindresorhus · 2018-07-06T19:15:39Z

A common use-case is just overriding a few defaults when creating a new instance. I don't like how you now need to specify a nested object to set a few options.

I think got.fork() should be optimized for the common-case and only allow overriding options, not handler/methods. If users need more advanced control, they can use got.create() and pass in the exposed got.defaults.

So got.fork():

const client = got.fork({headers: {'x-foo': 'bar'});

Also, is fork the best word for this? Some alternatives would be extend() or defaults(). Not saying it's not. Just putting it out there.

sindresorhus · 2018-07-06T19:54:59Z

source/as-stream.js

-	options.stream = true;
+module.exports = (url, options) => {
+	const normalizedArgs = normalizeArguments(url, options);
+	normalizedArgs.stream = true;


normalizedArgs => normalizedOptions would be more accurate, but that's quite verbose.

What's wrong with just:

options = normalizeArguments(url, options);

Do we need the original options somewhere?

Fixed that.

sindresorhus · 2018-07-07T09:23:57Z

readme.md

+#### got.create(settings)
+
+Configure a new `got` instance with provided settings:
+


Since we plan to use this for gh-got, I think we should just link to it as an example now.

sindresorhus · 2018-07-07T09:26:22Z

readme.md

+
+##### methods
+
+Array of supported methods.


methods => request methods

Maybe also document how to just use the defaults methods: methods: got.defaults.methods?

szmarczak · 2018-07-07T10:56:51Z

source/index.js

-	headers: {
-		'user-agent': `${pkg.name}/${pkg.version} (https://github.com/sindresorhus/got)`
+	handler: (url, options, next) => {
+		return next(url, options);


I think handler could use next directly. Like handler: next

szmarczak · 2018-07-07T10:57:49Z

source/create.js

-			}
-		}
+	if (options.endpoint) {
+		url = /^https?/.test(path) ? path : options.endpoint + path;


This should be:

if (options.endpoint && !/^https?/.test(path)) { url = options.endpoint + path; }

szmarczak · 2018-07-07T11:09:53Z

source/create.js

 		} catch (error) {
 			return Promise.reject(error);
 		}
 	}

-	got.create = (options = {}) => create(assignOptions(defaults, options));
+	got.create = newDefaults => create(newDefaults);


This should be:

got.create = create;

sindresorhus · 2018-07-07T11:48:38Z

source/create.js

-			}
-		}
+	if (options.endpoint && !/^https?/.test(path)) {
+		url = options.endpoint + path;


I think we should do url = (new URLGlobal(path, options.endpoint)).toString() so it handles things like options.endpoint ending in a slash and path starting with a slash. That would end up with https://foo.com//path here (Could you add a test for the specifically too?).

The above if (options.endpoint && !/^https?/.test(path)) { check can actually be simplified to just if (options.endpoint) { then.

Yeah, I was using that in my app but forgot about it :D

source/create.js

-				continue;
-			}
-		}
+	if (options.endpoint && !/^https?/.test(path)) {


sindresorhus · 2018-07-07T11:56:55Z

readme.md

@@ -99,6 +99,14 @@ Properties from `options` will override properties in the parsed `url`.

 If no protocol is specified, it will default to `https`.

+##### endpoint


I'm not sure about the word endpoint anymore. It is too much connected to REST APIs. This one is more generic. Maybe we should follow new URL() and just call it base or baseUrl? I prefer the latter.

sindresorhus · 2018-07-07T12:00:14Z

readme.md

+Type: `string` `Object`
+
+When specified, `url` will be preceded by `endpoint`.
+


Can you also mention that this one is especially useful with got.extend() to create niche specific Got instances?

sindresorhus · 2018-07-07T12:01:15Z

readme.md

+
+##### methods
+
+Type: `object`


sindresorhus · 2018-07-07T12:01:40Z

readme.md

+
+##### handler
+
+Type: `function`<br>


sindresorhus · 2018-07-07T12:02:02Z

readme.md

+Function making additional changes to the request.
+
+To inherit from parent, set it as `got.defaults.handler`.<br>
+To use the default handler, set it as `null` or `undefined`.


Couldn't this just be: To use the default handler, just omit specifying this.?

sindresorhus · 2018-07-07T12:03:13Z

readme.md

+
+##### [options](#options)
+
+To inherit from parent, set it as `got.defaults.options` or use [destructuring assignment](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment).


use [destructuring assignment]

Did you mean to say "object spread"?

Yep, naming is mixed up in my head sometimes

sindresorhus · 2018-07-07T12:03:54Z

readme.md

+
+###### [options](#options)
+
+###### next


next => next()

sindresorhus · 2018-07-07T12:09:02Z

Can you add a small tip to the Tips section (https://github.com/sindresorhus/got#tips) on how to use the endpoint option with got.extend() to quickly get an endpoint specific Got instance? I think that's a common need, so would be good to have a clear example for that specific thing.

sindresorhus · 2018-07-07T12:11:54Z

readme.md

+
+Example: [gh-got](https://github.com/sindresorhus/gh-got/blob/master/index.js)
+
+Configure a new `got` instance with provided settings:


We should be more explicit here about got.create() having no good defaults included by default and how it's different from got.extend().

brandon93s · 2018-07-07T17:12:02Z

I'm a bit concerned that a user is going to be confused as to whether they should be using get.extend() or got.create() for their use case. Most users are just going to want to provide new defaults, but extend has a small blurb in the docs compared to the in-depth create section which could mislead them. create seems to cater primarily to library / module creation (e.g. gh-got) and not everyday use. It may be time to consider breaking out some advanced sections of the docs into separate files and linking to them to avoid information overload:

Need more control over the behavior of got? Check out the advanced creation options.

Looking elsewhere, axios has axios.create() and request has request.defaults() which are functionally equivalent to got.extend(). Users coming from axios may reach for create and have a very different experience.

brandon93s · 2018-07-07T17:17:58Z

readme.md

+
+Type: `string` `Object`
+
+When specified, `url` will be preceded by `baseUrl`.<br>


"will be prepended with baseUrl" is a bit more clear.

Should we note that this only applies to relative URLs? If you specify an absolute URL it will skip the baseUrl.

The placement in the docs has this as a separate parameter to the got api. I think it's just part of the options, however. If that's the case it should be moved into the options section below.

Yes, it's an option. Done in next commit.

brandon93s · 2018-07-07T17:22:39Z

readme.md


-Configure a new `got` instance with default `options`:
+Configure a new `got` instance with default `options` and custom `baseUrl` (optional):


See other notes, baseUrl is an option. This feels redundant.

brandon93s · 2018-07-07T17:30:06Z

readme.md


-Configure a new `got` instance with default `options`:
+Configure a new `got` instance with default `options` and custom `baseUrl` (optional):

 ```js
 (async () => {


This example is a bit unclear if you're not familiar with httpbin.org and knowing that this endpoint will reflect headers back in the body. Maybe something like this would help illustrate:

const client = got.extend({ baseUrl: 'https://example.com', headers: { 'x-unicorn': 'rainbow' } }); client.get('/demo') /* HTTP Request => * GET /demo HTTP/1.1 * Host: example.com * x-unicorn: rainbow */

I know about httpbin.org but I didn't think of it. I'll change this to httpbin.org and provide the commented result :)

Oh wait, you mean users may not be familiar with httpbin.org? Then yes too. I'll replace that.

brandon93s · 2018-07-07T17:48:10Z

source/create.js

 		got[method] = (url, options) => got(url, {...options, method});
 		got.stream[method] = (url, options) => got.stream(url, {...options, method});
 	}

 	Object.assign(got, errors);
+	Object.assign(got, {defaults});


Exposing this and having it used in the closure could lead to some unexpected behavior if defaults are modified:

test('tampering with defaults', async t => { const instance = got.create({ handler: got.defaults.handler, methods: got.defaults.methods, options: { ...got.defaults.options, baseUrl: 'example' } }); const instance2 = instance.create({ handler: instance.defaults.handler, methods: instance.defaults.methods, options: instance.defaults.options }); // Tamper Time instance.defaults.options.baseUrl = 'http://google.com' t.is(instance.defaults.options.baseUrl, instance2.defaults.options.baseUrl); });

I think I'd expect two instances created with .create to be independent and not impact each other. Similar tampering could be performed on the global got.defaults that the module exports.

Thoughts?

You're right. We should deep freeze those or clone.

We can deep clone it with the extend dependency, I think.

Deep cloning may lead to false information because it's still writable. Used deep freeze.

sindresorhus · 2018-07-07T18:49:53Z

@brandon93s Good point. Let's move it out into a separate file like you've described.

szmarczak · 2018-07-07T19:34:14Z

@brandon93s I would leave the names unchanged. Previously got.extend() was named got.fork() (original creator named it got.create() but we need a real got.create() - no defaults). I would just note in readme.md/xxx.md the difference between got.create() and got.extend(). I can rename it anytime if it's needed.

sindresorhus · 2018-07-07T19:41:18Z

@szmarczak I don't think @brandon93s was suggesting we rename it, but rather just pointing out the benefit of moving got.create() out of the readme.

szmarczak · 2018-07-07T19:42:59Z

@sindresorhus So I'll do that tomorrow then 🦄

sindresorhus · 2018-07-07T19:43:35Z

source/create.js

+const deepFreeze = obj => {
+	const keys = Object.keys(obj);
+
+	for (let i = 0; i < keys.length; i++) {


Use a for-of loop and Object.entries().

Wouldn't it be better: for-of and Object.keys? I don't see much difference.

Object.entries() is better as it gives you the value too right away, instead of having to do options.headers[key].

sindresorhus · 2018-07-07T19:44:23Z

source/create.js

+	const keys = Object.keys(obj);
+
+	for (let i = 0; i < keys.length; i++) {
+		if (typeof obj[keys[i]] === 'object' && obj[keys[i]] !== null) {


Use the is module here: is.object

sindresorhus · 2018-07-08T11:47:05Z

advanced-creation.md

+
+> Make calling REST APIs easier by creating niche-specific `got` instances.
+
+#### got.extend([options])


got.extend() should stay in the readme, it was only got.create() that supposed to be moved out.

sindresorhus · 2018-07-08T13:14:31Z

This is perfect now. Amazing work @szmarczak 🙌

Simplify wrapping got

4bc7e21

szmarczak mentioned this pull request Jul 6, 2018

Endpoint prefix / default config #203

Closed

szmarczak and others added 3 commits July 6, 2018 15:37

Shorter url argument

3c6e142

Fix

954e79c

Split create into fork & create

71d00f4

Update readme.md

b9cd9ab

szmarczak added 6 commits July 6, 2018 21:34

Don't expose many things

9fa97d7

Merge branch 'got-api' of https://github.com/szmarczak/got into got-api

95547d7

Update readme.md

b097ed2

No more public access to normalizeArguments

2e9589e

readme.md: got.extend() before got.create()

151d5ec

No unused normalizeArguments value

6a7d77e

sindresorhus reviewed Jul 6, 2018

View reviewed changes

szmarczak and others added 2 commits July 7, 2018 10:54

Don't expose asStream & asPromise

fc0c4a6

Update readme.md

e0fde4e

sindresorhus reviewed Jul 7, 2018

View reviewed changes

szmarczak added 2 commits July 7, 2018 12:19

Update readme.md, add endpoint option

21664a5

Move endpoint into options

87e0fb1

szmarczak commented Jul 7, 2018

View reviewed changes

szmarczak and others added 2 commits July 7, 2018 13:20

Optional handler; update readme.md

d6d67bb

Update readme.md

7fa917a

sindresorhus requested changes Jul 7, 2018

View reviewed changes

sindresorhus reviewed Jul 7, 2018

View reviewed changes

Rename endpoint to baseUrl

aa94bed

brandon93s suggested changes Jul 7, 2018

View reviewed changes

sindresorhus force-pushed the master branch from 448772c to 57ff833 Compare July 7, 2018 18:46

Update readme.md, freeze got.defaults

d5d3136

sindresorhus reviewed Jul 7, 2018

View reviewed changes

sindresorhus and others added 6 commits July 8, 2018 02:47

Update readme.md

c43d703

Update readme.md

bd2bd9f

Update readme.md

dafebc9

Split deep-freeze into a different file

bd2b78d

Create advanced-creation.md

9fc6269

Update readme.md

721e6db

sindresorhus reviewed Jul 8, 2018

View reviewed changes

szmarczak and others added 4 commits July 8, 2018 14:27

Move got.extend() to readme.md

1ec0c4a

Merge branch 'got-api' of https://github.com/szmarczak/got into got-api

7cf71a3

Update readme.md

4cb1929

Update advanced-creation.md

4dd9fd7

sindresorhus merged commit bc41a49 into sindresorhus:master Jul 8, 2018

szmarczak deleted the got-api branch January 17, 2019 18:54

		#### got.create(settings)

		Configure a new `got` instance with provided settings:

		@@ -99,6 +99,14 @@ Properties from `options` will override properties in the parsed `url`.

		If no protocol is specified, it will default to `https`.

		##### endpoint

		Type: `string` `Object`

		When specified, `url` will be preceded by `endpoint`.


		##### [options](#options)

		To inherit from parent, set it as `got.defaults.options` or use [destructuring assignment](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment).


		Example: [gh-got](https://github.com/sindresorhus/gh-got/blob/master/index.js)

		Configure a new `got` instance with provided settings:


		Configure a new `got` instance with default `options`:
		Configure a new `got` instance with default `options` and custom `baseUrl` (optional):


		> Make calling REST APIs easier by creating niche-specific `got` instances.

		#### got.extend([options])


		###### [options](#options)

		###### next

Simplify wrapping got #503

Simplify wrapping got #503

Conversation

szmarczak commented Jul 6, 2018 • edited

sindresorhus commented Jul 6, 2018

sindresorhus commented Jul 6, 2018

sindresorhus commented Jul 6, 2018

szmarczak commented Jul 6, 2018

sindresorhus commented Jul 6, 2018

sindresorhus commented Jul 6, 2018

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

szmarczak Jul 7, 2018 • edited

Choose a reason for hiding this comment

szmarczak Jul 7, 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

This comment was marked as outdated.

This comment was marked as outdated.

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 Jul 7, 2018

Choose a reason for hiding this comment

brandon93s commented Jul 7, 2018

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 Jul 7, 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

sindresorhus commented Jul 7, 2018

szmarczak commented Jul 7, 2018 • edited

sindresorhus commented Jul 7, 2018

szmarczak commented Jul 7, 2018

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 Jul 8, 2018

Simplify wrapping `got` #503

Simplify wrapping `got` #503

szmarczak commented Jul 6, 2018 •

edited

szmarczak Jul 7, 2018 •

edited

szmarczak Jul 7, 2018 •

edited

szmarczak Jul 7, 2018 •

edited

szmarczak commented Jul 7, 2018 •

edited