Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support discrete client configuration via got.create([options]) #495

Merged
merged 3 commits into from
Jul 5, 2018

Conversation

knksmith57
Copy link
Contributor

Add first-class support for discrete client configuration via new public got.create([options]) method.

Feature is 100% backwards compatible and internal defaults have been refactored ever so slightly to dogfood the interface.

I did my best to adhere to existing code standards-- linted w/ xo, added tests to maintain coverage (statements + lines are slightly up, branches are slightly down), documented the method in readme.md, and tailored the commit message style to match the predominant pattern.

My first time contributing here so please let me know if there's anything else I can do to help! If y'all prefer to keep this out of core I'll understand and can publish elsewhere-- no hard feelings either way.

Thanks!

@knksmith57
Copy link
Contributor Author

/cc @jstewmon in case you wanted to take a pass at this

function create(defaults = {}) {
function got(url, opts) {
try {
opts = extend(true, {}, defaults, opts);
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the global defaults are not inherited? Was that intentional? My expectation would be that when I create a client, the global defaults are inherited.

I think you can easily address by renaming the function argument to clientDefaults and making this line:

opts = extend(true, {}, defaults, clientDefaults, opts);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the global defaults are inherited as the top-level got export dogfoods this interface and all public create() calls operate off of that instance (or a descendant of it):

const got = create(defaults);  // L43

Copy link
Contributor Author

Choose a reason for hiding this comment

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

heads up that, unless I'm missing something, this should be covered by the preserve global defaults test case in test/create.js starting on line 18

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I see now that got.create is assigned at the end of this method, so that the exported got has create which uses the closure binding to preserve the global defaults.

@ewolfe
Copy link

ewolfe commented Jul 1, 2018

I just pulled this down and it works great :)

I found myself wanting to create an instance with a hostname option and using only the path as the argument to my instance. ie:

const client = got.create({
  hostname: 'https://example.com',
  path: '/api',
  headers: {
    Authorization: `Bearer ${TOKEN}`,
  },
});

const response = await client('/hello-world'); // GET https://example.com/api/hello-world

What are your thoughts on adding that functionality? I don't think it's too opinionated and is in the spirit of the tagline "Simplified HTTP requests"

@jstewmon
Copy link
Contributor

jstewmon commented Jul 1, 2018

@ewolfe that feature is usually implemented with a discrete option baseUrl to disambiguate the caller's intent. I'd say that's a complementary feature to being able to create an instance, but deserving of its own PR, since it would add a config param.

@knksmith57
Copy link
Contributor Author

@ewolfe thanks for taking a look! Super glad you find the feature useful.

re simplified interface: I would love to get there with got and have a few ideas on how to support that sort of functionality without polluting the public API-- @jstewmon's suggestion to support something like baseUrl is definitely one way to do it.

For what it's worth, everything from your example snippet except the URI prefixing is made possible by this feature (protocol + hostname + default headers).

If there's interest, I'd be happy to put together a proposal PR. Thanks again for taking a look!

@knksmith57
Copy link
Contributor Author

how do y'all feel about this @sindresorhus / @lukechilds / @brandon93s?

@lukechilds
Copy link
Sponsor Contributor

lukechilds commented Jul 3, 2018

I'm quite busy no another project right now so I haven't reviewed the code in depth, but I really like the idea.

Much nicer than having to manually create a wrapper function and should make writing API clients with Got simpler too.

@sindresorhus
Copy link
Owner

This is a nice feature. I'll take a closer look this weekend and merge.

@sindresorhus sindresorhus merged commit 058452b into sindresorhus:master Jul 5, 2018
@sindresorhus
Copy link
Owner

This looks great 🙌

It will probably be at least a week until I can publish this as I'm currently preparing for a big v9 release.

@sindresorhus
Copy link
Owner

I found myself wanting to create an instance with a hostname option and using only the path as the argument to my instance. ie:

@ewolfe That would be very handy. Can you open a new issue about it?

@knksmith57 knksmith57 deleted the support-instances branch July 11, 2018 20:35
@szmarczak szmarczak mentioned this pull request Nov 12, 2019
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants