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

Custom options #1794

Closed
szmarczak opened this issue Jul 20, 2021 · 11 comments
Closed

Custom options #1794

szmarczak opened this issue Jul 20, 2021 · 11 comments
Labels
documentation The issue will improve the docs ✭ help wanted ✭

Comments

@szmarczak
Copy link
Collaborator

Got v12 throws when an option does not exist. So the workaround is:

	hooks: {
		init: [
			(raw, options) => {
				if ('token' in raw) {
					options.context.token = raw.token;
					delete raw.token;
				}
			}
		]
	},

this way got('...', {token: 'secret'}) will set options.context.token. I think there should be a dedicated API for custom options. Maybe this:

const instance = got.extend({
	get example() {
		// `this` returns a store (an object)
		return this.example;
	},
	set example(value) {
		// `this` returns a store (an object)
		this.example = value;
	}
});

or this:

import got from 'got';

class MyOptions extends Options {
	get example() {
		return this._example;
	}

	set example(value) {
		this._example = value;
	}
}

const instance = got.extend({Options: MyOptions});

or both?

/cc @sindresorhus @mnmkng

@szmarczak szmarczak added enhancement This change will extend Got features ✭ help wanted ✭ labels Jul 20, 2021
@mnmkng
Copy link
Contributor

mnmkng commented Jul 20, 2021

We were thinking about asking for a feature like this, but in the end decided not to bother you with this. So yeah, we would appreciate it 😁

I think extending the Options is a more intuitive interface. I would not be sure what the plain extend with getters and setters does without reading the docs.

@szmarczak
Copy link
Collaborator Author

but in the end decided not to bother you with this.

No, please, you can ask me anything, I'm more than happy to discuss API improvements :)

I think extending the Options is a more intuitive interface. I would not be sure what the plain extend with getters and setters does without reading the docs.

I'm thinking the same. This way you can also easily validate the options like this:

got/source/core/options.ts

Lines 934 to 935 in d44b2d2

set agent(value: Agents) {
assert.plainObject(value);

@szmarczak szmarczak pinned this issue Jul 22, 2021
@szmarczak
Copy link
Collaborator Author

szmarczak commented Jul 22, 2021

One of the problems is also how to merge custom instances with this. So extending the Options class would be quite problematic.

The first example is counterintuitive as getters are used to retrieve the value, and Got wouldn't know if the getter should be copied to Options or it's just a regular value that Got should execute.

So we can solve this in two ways:

const instance = got.extend({
    custom: {
        foobar: prototypeObject
    }
});
const instance = got.extend({
    custom: ['foobar']
});

To be honest I prefer the last approach. It's the easiest to use, can be easily merged and the normalisation (if necessary) can happen in the init hooks.

@rightaway
Copy link

@szmarczak Are all the options in https://nodejs.org/api/http.html#http_http_request_url_options_callback supported by got or any are incompatible? I remember a note about this in the docs but can't find it anymore in https://github.com/sindresorhus/got/blob/main/documentation/2-options.md.

@szmarczak
Copy link
Collaborator Author

Got doesn't support insecureHTTPParser and never will. signal is a to-do.

@rightaway
Copy link

@szmarczak Apart from those 2 options, all the other ones in https://nodejs.org/api/http.html#http_http_request_url_options_callback can be provided as got options and they will be passed through to the underlying node http.request?

@szmarczak
Copy link
Collaborator Author

Some of them yes, some have been renamed, some accept different types. If you're looking for 1:1 then this is not Got.

@mnmkng
Copy link
Contributor

mnmkng commented Jul 25, 2021

const instance = got.extend({
    custom: ['foobar']
});

I also think this is a good approach. I assume it will only override the default validation i.e. the above is equivalent to saying "Do not throw when you see a foobar option." and nothing else. Correct?

@szmarczak
Copy link
Collaborator Author

Correct. The full solution is already in #1796 but it requires some hilarious TypeScript hacks, it's just not pretty on the TypeScript side but it works as expected (the end types are correct too). I'm ok with it anyway.

Eventually we could default all non-Got options to be stored inside options.context.

See sindresorhus/type-fest#232

@rightaway
Copy link

If you're looking for 1:1 then this is not Got.

@szmarczak Not looking for 1:1, the Got interface is nicer.

Some of them yes, some have been renamed, some accept different types.

Could these differences be added to the options documentation? It could help people migrating to Got and even existing users to see a list of all http.request options and how they map to Got.

@szmarczak
Copy link
Collaborator Author

Umm... the list would be quite long. It would be better if you'd refer to the documentation directly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation The issue will improve the docs ✭ help wanted ✭
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants