Skip to content
This repository has been archived by the owner on Mar 7, 2021. It is now read-only.

Fix deleting requestOptions.port when blank/no port set #357

Merged
merged 2 commits into from
Mar 17, 2017

Conversation

stilliard
Copy link
Contributor

What this PR changes

Deletes requestOptions.port when default blank / no port returned, same as it does when port 80 or 443 specified.

Rationale

Currently the getRequestOptions() method in crawler.js deletes the port setting when it's set to a default 80 or 443, but I noticed when connecting in a different https agent that the default port is often blank which should also be deleted here I believe.

This then fixes also an issue an issue I experienced when using proxy-agent as the httpAgent & httpsAgent where for HTTPS requests the port would end up as 80 when passed over as blank.

Hope this makes sense, thanks.

@fredrikekelund
Copy link
Collaborator

Hi, @stilliard! Thanks for filing a PR. Sure, this makes sense to me. Would we cover any additional edge cases by doing a more lax comparison, like if (requestPort === 80 || requestPort === 443 || !requestPort) { instead of if (requestPort === 80 || requestPort === 443 || requestPort === "") {?

@stilliard
Copy link
Contributor Author

Hi @fredrikekelund! Sure, I think it shouldn't be a problem as servers can't bind to port 0 anyway.
& It'll be a little safer in case in the future the port comes back as null or false instead of a blank string.
I can make the change in the pull request now if you'd like?

@fredrikekelund
Copy link
Collaborator

That would be great, I can go ahead and merge this after you've made the change!

@stilliard
Copy link
Contributor Author

Sure no problem, that change has now been made.

@fredrikekelund fredrikekelund merged commit d80eefd into simplecrawler:master Mar 17, 2017
@fredrikekelund
Copy link
Collaborator

Thanks @stilliard!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants