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

http: make globalAgent of http and https overridable #11249

Closed
wants to merge 1 commit into from
Closed

http: make globalAgent of http and https overridable #11249

wants to merge 1 commit into from

Conversation

shubheksha
Copy link
Contributor

Previously globalAgent of http and https cannot be overriden by simply assigning a new value. This exposes those properties to allow overriding by assignment.

Fixes #9057

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

http

Previously `globalAgent` of `http` and `https` cannot be overriden by simply assigning a new value. This exposes those properties to allow overriding by assignment.
@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. labels Feb 8, 2017
if (!options.hostname) {
throw new Error('Unable to determine the domain name');
}
} else if (options instanceof url.URL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

/cc @jasnell Is there a faster method we could use to check for this instead of using instanceof? Like checking for a symbol or something?

Copy link
Contributor

@mscdex mscdex Feb 8, 2017

Choose a reason for hiding this comment

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

Actually, it would be best if we could avoid duplicating all of these options checks, since they're already being done in ClientRequest(). But it would still be nice to avoid instanceof in ClientRequest().

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mscdex, I didn't want to duplicate the options check too, but w/o the tests with options of type string fail as _defaultAgent cannot be added as a property on it. Instead of this, should I just check if type of options is Object or not? Is there another workaround?

Copy link
Contributor

@mscdex mscdex Feb 8, 2017

Choose a reason for hiding this comment

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

typeof options === 'object' && options !== null won't work since that would return true for url.URL instances as well.

Copy link
Member

Choose a reason for hiding this comment

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

@mscdex ... you could export the context symbol from internal/url and check for that. All URL instances would have that set internally.

Copy link
Contributor

@mscdex mscdex Feb 17, 2017

Choose a reason for hiding this comment

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

+1 to the Symbol checking solution then

@Fishrock123 Fishrock123 added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 8, 2017
@Fishrock123
Copy link
Contributor

I think this is a feature addition, so, tagging as semver-minor.

@gergelyke
Copy link
Contributor

I guess this can be closed, seems to be fixed on master

@mscdex
Copy link
Contributor

mscdex commented May 18, 2017

@gergelyke It's not fixed for https.request().

@BridgeAR
Copy link
Member

This needs a rebase

@BridgeAR
Copy link
Member

Ping @shubheksha do you want to follow up on this?

@BridgeAR
Copy link
Member

Closing this due to the long inactivity.

@shubheksha I am sorry this could not land as is. Your work is much appreciated nevertheless! If you would like to pursue this further, please leave a comment or open a new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. https Issues or PRs related to the https subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify https.globalAgent doesn't effect
7 participants