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: replace util._extend() with Object.create() #592

Closed
wants to merge 1 commit into from
Closed

http: replace util._extend() with Object.create() #592

wants to merge 1 commit into from

Conversation

jonathanong
Copy link
Contributor

options is never touched, so there's no need to make a copy. Anyways, a better alternative for making copies is to just Object.create() the object. An alternative solution is to not bother copying at all.

This solves issues such as https://github.com/petkaantonov/urlparser/issues where the options object is created from a constructor.

`options` is never touched, so there's no need to make a copy. Anyways, a better alternative for making copies is to just `Object.create()` the object. An alternative solution is to not bother copying at all. 

This solves issues such as https://github.com/petkaantonov/urlparser/issues where the `options` object is created from a constructor.
@vkurchatkin
Copy link
Contributor

-1 on Object.create for options in general. options should be treated as maps in general. too many hasOwnProperty used on options all over the place

Qard pushed a commit that referenced this pull request Jan 24, 2015
Alternative to #592. The `options` object is never overwritten, so making a copy is not necessary.

This solves issues such as https://github.com/petkaantonov/urlparser/issues where the options object is created from a constructor.

PR-URL: #593
Reviewed-By: Stephen Belanger <[email protected]>
@jonathanong
Copy link
Contributor Author

the other one was merged anyways, which i preferred. 👍

@jonathanong jonathanong deleted the patch-1 branch January 24, 2015 21:12
brendanashworth referenced this pull request May 8, 2015
This reverts commit 06cfff9.

Reverted because it introduced a regression where (because options were
modified in the later functionality) options.host and options.port would
be overridden with values provided in other, supported ways.

PR-URL: #1467
Reviewed-By: Ben Noordhuis <[email protected]>
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.

2 participants