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

Code cleanup and pathname issue #77

Merged
merged 3 commits into from
Jun 27, 2015
Merged

Code cleanup and pathname issue #77

merged 3 commits into from
Jun 27, 2015

Conversation

floatdrop
Copy link
Contributor

There were some 💩 code (by me) like this:

arg.path = (arg.path ? arg.path.split('?')[0] : '') + '?' + (typeof query === 'string' ? query : querystring.stringify(query));

Or this:

var parsedUrl = typeof url === 'string' ? urlLib.parse(prependHttp(url)) : url;
// ...
url = typeof url === 'string' ? prependHttp(url) : urlLib.format(url);

Which was hard to maintain and read.

Main reason for this code - there was two different arguments for get function - first contains request options (as object or as string) and other gets got options.

This logic was replaced by merging url and opts arguments in got function into one. Because (on redirect) we can reassign parsed request options.

In the process redirect event was fixed - it was emitting old opts that was not from redirect itself.

#72 was fixed by reassing path to pathname (if pathname is present). I don't know, if this is right thing to do, but because of difference in http.request and url.format + url.parse we have to normalise options or use another url.format implementation.

//cc @sindresorhus @sindresorhus @kevva

Parse url at the top of got function and work with request object all the way down.
This reduces complexity and unnecessary delete calls in redirects.
@floatdrop
Copy link
Contributor Author

Side-effect: now you can specify got options in first argument:

got({host: 'google.com', timeout: 2000});

@sindresorhus
Copy link
Owner

Side-effect: now you can specify got options in first argument:

Should this be documented?

@sindresorhus
Copy link
Owner

Super nice cleanup and improvements @floatdrop :D

sindresorhus added a commit that referenced this pull request Jun 27, 2015
@sindresorhus sindresorhus merged commit ee6b3ad into master Jun 27, 2015
@sindresorhus sindresorhus deleted the path-vs-pathname branch June 27, 2015 22:50
@sindresorhus
Copy link
Owner

tumblr_ml9za9ecsb1qdlh1io1_400

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

2 participants