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

Add migration guides #595

Merged
merged 15 commits into from
Dec 4, 2018
Merged

Add migration guides #595

merged 15 commits into from
Dec 4, 2018

Conversation

szmarczak
Copy link
Collaborator

@szmarczak szmarczak commented Aug 29, 2018

No description provided.

@sindresorhus
Copy link
Owner

I think each migration should be a separate file.

migration-guides.md Outdated Show resolved Hide resolved
@szmarczak
Copy link
Collaborator Author

This PR is not my first priority right now, so it may take a while to finish this.

migration-guides.md Outdated Show resolved Hide resolved
migration-guides.md Outdated Show resolved Hide resolved
migration-guides.md Outdated Show resolved Hide resolved
migration-guides.md Outdated Show resolved Hide resolved
migration-guides.md Outdated Show resolved Hide resolved
migration-guides.md Outdated Show resolved Hide resolved
migration-guides.md Outdated Show resolved Hide resolved
migration-guides.md Outdated Show resolved Hide resolved
migration-guides.md Show resolved Hide resolved
- No `oauth`/`hawk`/`aws`/`httpSignature` option. To sign requests, you need to create a [custom instance](advanced-creation.md#signing-requests).
- No `agentClass`/`agentOptions`/`forever`/`pool` option.
- No proxy option. You need to [pass a custom agent](readme.md#proxies).
- No `removeRefererHeader` option. You can remove the referer header in a [`beforeRequest` hook](https://github.com/sindresorhus/got#hooksbeforeRequest):
Copy link
Owner

Choose a reason for hiding this comment

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

Also no maxRedirects option. There are many more too that are not listed here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I prefer to operate on parents, not children (options) :P

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's more clear.

Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to operate on parents, not children (options)

What does that mean?

Copy link
Collaborator Author

@szmarczak szmarczak Nov 1, 2018

Choose a reason for hiding this comment

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

followRedirect - follow HTTP 3xx responses as redirects (default: true). This property can also be implemented as function which gets response object as a single argument and should return true if redirects should continue or false otherwise.
followAllRedirects - follow non-GET HTTP 3xx responses as redirects (default: false)
followOriginalHttpMethod - by default we redirect to HTTP method GET. you can enable this property to redirect to the original HTTP method (default: false)

It's counted as only one option: followRedirect. (followAllRedirects and followOriginalHttpMethod are the children of followRedirect, 'ya know what I mean?)

Copy link
Collaborator Author

@szmarczak szmarczak Nov 1, 2018

Choose a reason for hiding this comment

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

Also, some Request's options are not spec-compliant.

Copy link
Owner

Choose a reason for hiding this comment

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

It's counted as only one option: followRedirect. (followAllRedirects and followOriginalHttpMethod are the children of followRedirect, 'ya know what I mean?)

Yes, but is that clear enough to the end-user though? Why not be explicit about it? Some might just do Cmd+F to try to find what they're looking for. I think we should list them all to make it easy to the user to discover.

Also, some Request's options are not spec-compliant.

Spec-compliant to what? HTTP? And does it matter? Then we list it and say that Got does not support that because it's not spec compliant.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I did not say we should have no such feature because it isn't spec-compliant. I meant Got's more spec-compliant than Request. Anyway, nevermind. It doesn't matter. I'll just list them.

Copy link
Owner

Choose a reason for hiding this comment

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

I got (pun intended) what you meant. I just think we should list it no matter what.

migration-guides.md Outdated Show resolved Hide resolved

- No `form` option. You have to pass a [`form-data` instance](https://github.com/form-data/form-data) through the [`body` option](https://github.com/sindresorhus/got#body).
- No `oauth`/`hawk`/`aws`/`httpSignature` option. To sign requests, you need to create a [custom instance](advanced-creation.md#signing-requests).
- No `agentClass`/`agentOptions`/`forever`/`pool` option.
Copy link
Owner

Choose a reason for hiding this comment

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

Can we provide any transition steps / alternatives for these options?

Copy link
Collaborator Author

@szmarczak szmarczak Nov 1, 2018

Choose a reason for hiding this comment

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

agent: new agentClass(agentOptions)

forever-agent

pool implementation:

const agentSymbol = Symbol('agent');
const pools = new Map();

const getKeyFromPool = pool => ...; // TODO

const getAgent = (pool, options) => {
	const merged = {...pool, ...options};
	const key = getKeyFromPool(merged);

	if (!pools.has(key)) {
		pools.set(key, new specifiedAgentByUserOrDefault(pool));
	}

	return pools.get(key);
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

will do :)

@sindresorhus
Copy link
Owner

Can you link to this at the top of the readme with a link text like:

Moving from Request?

migration-guides.md Outdated Show resolved Hide resolved
migration-guides.md Outdated Show resolved Hide resolved
@sindresorhus sindresorhus changed the title [WIP] Migration guides Add migration guides Nov 29, 2018
],
afterResponse: [
response => {
// TODO in Got: we need to make the `options` public somehow
Copy link
Owner

Choose a reason for hiding this comment

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

@szmarczak Can you open a new issue about this?

@sindresorhus
Copy link
Owner

Looking good :)

Still some minor things left, but these things can be done in a follow-up PR:

@sindresorhus sindresorhus merged commit 8848a7a into sindresorhus:master Dec 4, 2018
@szmarczak szmarczak deleted the guides branch January 17, 2019 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants