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.

Sign up for GitHub

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

Jump to bottom

http: revert 06cfff93, fixes overridden options #1467

Closed
wants to merge 4 commits into from
Closed

http: revert 06cfff93, fixes overridden options #1467

wants to merge 4 commits into from

Conversation

brendanashworth
Copy link
Contributor

This commit reverts commit 06cfff9 and adds a test that failed before
the revert. Options is in fact overridden in the code below the
util._extend() call (specifically options.port and options.host) to
allow for a one-argument net.createConnection(options) call.

Alternatives

Since this commit could be semver-major as it reverts functionality that's been there for a while, even though the parent commit introduced a regression that should've been semver-major too, we should look into alternatives. These are some I see:

  • say not all arguments passed to stdlib functions will be treated as immutable
  • for _http_client.js, handle createConnection differently (I already have different functionality we could try in a separate branch I haven't PR'ed yet). I'm 👍 on this functionality.

cc @jonathanong
ref #62 #593

@brendanashworth brendanashworth added the http Issues or PRs related to the http subsystem. label Apr 18, 2015
bnoordhuis
bnoordhuis reviewed Apr 18, 2015
View reviewed changes
test/parallel/test-http-dont-override-options.js
@@ -0,0 +1,43 @@
const common = require('../common');
Copy link
Member

Choose a reason for hiding this comment

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

I thought const didn't work in sloppy mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't originally error for me, but I also thought so too so I added 'use strict'; in the new commit.

@bnoordhuis
Copy link
Member

Would it be possible to split this in git revert with an appropriate commit log (see e.g. 0526d83) and add the regression test in a follow-up commit? Having an 'official' revert commit log makes it easier to grep for them in the history.

@brendanashworth
Revert "http: don't bother making a copy of the options"
cd7023c 
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.
@brendanashworth
test: add test for 06cfff9 regression
8bccb49 
This commit adds a test to ensure all options are NOT modified after
passing them to http.request. Specifically options.host and options.port
are the most prominent that would previously error, but add the other
options that have default values.

options.host and options.port were overridden for the one-argument
net.createConnection(options) call.
@brendanashworth
!fixup fix test's localhost for FreeBSD runs
ff88a4f
@brendanashworth
!fixup more verbose test name
6e5cc0a
@brendanashworth
Copy link
Contributor Author

Updated for your comments @bnoordhuis. The revert commit has a title of longer than 50 chars now, I hope that is ok?

@bnoordhuis
Copy link
Member

Yes, revert commits get dispensation. LGTM.

@brendanashworth
Copy link
Contributor Author

Is this okay as semver-patch?

@bnoordhuis
Copy link
Member

I would say so. It fixes a regression.

@brendanashworth
Copy link
Contributor Author

Awesome, merging.

@brendanashworth brendanashworth self-assigned this Apr 18, 2015
brendanashworth added a commit that referenced this pull request Apr 18, 2015
@brendanashworth
Revert "http: don't bother making a copy of the options"
7180597 
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 <info@bnoordhuis.nl>
brendanashworth added a commit that referenced this pull request Apr 18, 2015
@brendanashworth
test: add test for 06cfff9 regression
6bf85bc 
This commit adds a test to ensure all options are NOT modified after
passing them to http.request. Specifically options.host and options.port
are the most prominent that would previously error, but add the other
options that have default values.

options.host and options.port were overridden for the one-argument
net.createConnection(options) call.

PR-URL: #1467
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@brendanashworth
Copy link
Contributor Author

Thanks, this was merged in 7180597 and 6bf85bc.

@brendanashworth brendanashworth removed their assignment Apr 18, 2015
@jonathanong
Copy link
Contributor

¯\_(ツ)_/¯

@jonathanong
Copy link
Contributor

personally i would at least change it to options = Object.create(options || {}) but i think other people care about inheritance

@Fishrock123
Copy link
Member

.. isn't this a breaking change though?

I guess it landed on master, but still. Weekend patches.

@rlidwka
Copy link
Contributor

rlidwka commented Apr 19, 2015

personally i would at least change it to options = Object.create(options || {})

+1

@brendanashworth
Copy link
Contributor Author

@jonathanong maybe reopen #592?

@jonathanong
Copy link
Contributor

sure

@jonathanong
Copy link
Contributor

nope... too many tests are failing for me.

@rvagg rvagg mentioned this pull request Apr 27, 2015
Closed
@brendanashworth brendanashworth deleted the http-dont-copy-options branch May 5, 2015 03:09
@brendanashworth brendanashworth mentioned this pull request Jul 23, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
http Issues or PRs related to the http subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@brendanashworth @bnoordhuis @jonathanong @Fishrock123 @rlidwka