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: add doc deprecation for private props #10941

Merged
merged 4 commits into from Mar 9, 2017

Conversation

@mscdex
Contributor

mscdex commented Jan 21, 2017

This is one proposed solution to the current backwards compatibility issue (see #10558) in master with end users who directly access outgoingMessage._headers/outgoingMessage._headerNames.

This PR will be simplified some once #10805 is landed (which should land before this PR). Specifically, the ._headers getter can simply just return this.getHeaders().

/cc @nodejs/collaborators

CI: https://ci.nodejs.org/job/node-test-pull-request/6054/

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
  • http
Show outdated Hide outdated lib/_http_client.js
@@ -182,7 +182,7 @@ function ClientRequest(options, cb) {
'client');
}
self._storeHeader(self.method + ' ' + self.path + ' HTTP/1.1\r\n',
self._headers);
self._headersStore);

This comment has been minimized.

@targos

targos Jan 21, 2017

Member

Maybe use a Symbol to hide it more?

@targos

targos Jan 21, 2017

Member

Maybe use a Symbol to hide it more?

This comment has been minimized.

@mscdex

mscdex Jan 21, 2017

Contributor

Does accessing a value via a Symbol have a perf hit vs. normal property access in master?

@mscdex

mscdex Jan 21, 2017

Contributor

Does accessing a value via a Symbol have a perf hit vs. normal property access in master?

This comment has been minimized.

@targos

targos Jan 21, 2017

Member

I don't know. I'll look into making a benchmark.

@targos

targos Jan 21, 2017

Member

I don't know. I'll look into making a benchmark.

This comment has been minimized.

@targos

targos Jan 21, 2017

Member

Results with the following benchmark (is it worth adding to the repo?): targos@d50c790

# master
$ ./node benchmark/es/object-property-bench.js
es/object-property-bench.js millions=1000 method="property": 1,137.5161172810194
es/object-property-bench.js millions=1000 method="string": 1,133.78613592118
es/object-property-bench.js millions=1000 method="variable": 1,137.9577172760735
es/object-property-bench.js millions=1000 method="symbol": 1,133.090467628974

# v7.4.0
$ node benchmark/es/object-property-bench.js
es/object-property-bench.js millions=1000 method="property": 1,136.7741112855451
es/object-property-bench.js millions=1000 method="string": 1,136.7865674716409
es/object-property-bench.js millions=1000 method="variable": 1,135.1437992275612
es/object-property-bench.js millions=1000 method="symbol": 1,136.4064052769731

# v4.7.1
$ node benchmark/es/object-property-bench.js
es/object-property-bench.js millions=1000 method="property": 1,135.4850535131566
es/object-property-bench.js millions=1000 method="string": 1,134.4381133016345
es/object-property-bench.js millions=1000 method="variable": 78.62998231306928
es/object-property-bench.js millions=1000 method="symbol": 81.82481813228071
@targos

targos Jan 21, 2017

Member

Results with the following benchmark (is it worth adding to the repo?): targos@d50c790

# master
$ ./node benchmark/es/object-property-bench.js
es/object-property-bench.js millions=1000 method="property": 1,137.5161172810194
es/object-property-bench.js millions=1000 method="string": 1,133.78613592118
es/object-property-bench.js millions=1000 method="variable": 1,137.9577172760735
es/object-property-bench.js millions=1000 method="symbol": 1,133.090467628974

# v7.4.0
$ node benchmark/es/object-property-bench.js
es/object-property-bench.js millions=1000 method="property": 1,136.7741112855451
es/object-property-bench.js millions=1000 method="string": 1,136.7865674716409
es/object-property-bench.js millions=1000 method="variable": 1,135.1437992275612
es/object-property-bench.js millions=1000 method="symbol": 1,136.4064052769731

# v4.7.1
$ node benchmark/es/object-property-bench.js
es/object-property-bench.js millions=1000 method="property": 1,135.4850535131566
es/object-property-bench.js millions=1000 method="string": 1,134.4381133016345
es/object-property-bench.js millions=1000 method="variable": 78.62998231306928
es/object-property-bench.js millions=1000 method="symbol": 81.82481813228071

This comment has been minimized.

@mscdex

mscdex Jan 21, 2017

Contributor

Ok so it might be worth sticking to properties for now.

As far as adding the benchmark, it could be useful. Although I'd probably put it in misc/ instead of es/ since it doesn't primarily deal with es6+ alternatives (which es/ seems to contain).

@mscdex

mscdex Jan 21, 2017

Contributor

Ok so it might be worth sticking to properties for now.

As far as adding the benchmark, it could be useful. Although I'd probably put it in misc/ instead of es/ since it doesn't primarily deal with es6+ alternatives (which es/ seems to contain).

This comment has been minimized.

@targos

targos Jan 22, 2017

Member

it might be worth sticking to properties for now

I don't think so. The numbers are very close and on multiple runs, the order varies a lot. It would be worth only for v4.x

@targos

targos Jan 22, 2017

Member

it might be worth sticking to properties for now

I don't think so. The numbers are very close and on multiple runs, the order varies a lot. It would be worth only for v4.x

This comment has been minimized.

@mcollina

mcollina Jan 22, 2017

Member

@targos @mscdex this matches my findings. In practice I did not see any perf impact in using symbols since v7.

@mcollina

mcollina Jan 22, 2017

Member

@targos @mscdex this matches my findings. In practice I did not see any perf impact in using symbols since v7.

This comment has been minimized.

@jasnell

jasnell Jan 23, 2017

Member

I've been testing this like crazy lately and from everything I have seen there is essentially zero perf impact from using symbols.

@jasnell

jasnell Jan 23, 2017

Member

I've been testing this like crazy lately and from everything I have seen there is essentially zero perf impact from using symbols.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 23, 2017

Member

+1 to using a symbol instead.

Member

jasnell commented Jan 23, 2017

+1 to using a symbol instead.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

thefourtheye Jan 28, 2017

Contributor

Isn't this a major change?

Contributor

thefourtheye commented Jan 28, 2017

Isn't this a major change?

@mscdex mscdex referenced this pull request Feb 1, 2017

Merged

http: add new functions to OutgoingMessage #10805

4 of 4 tasks complete

@addaleax addaleax added this to the 8.0.0 milestone Feb 15, 2017

@mscdex mscdex referenced this pull request Feb 19, 2017

Merged

module: improve require() performance #10789

3 of 3 tasks complete

@ChALkeR ChALkeR self-requested a review Feb 19, 2017

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 19, 2017

Contributor

This PR needs at least one more approval from a @nodejs/ctc member in order for it to land (again, after #10805 lands).

Contributor

mscdex commented Feb 19, 2017

This PR needs at least one more approval from a @nodejs/ctc member in order for it to land (again, after #10805 lands).

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 20, 2017

Contributor

Updated PR to use one of the new http API methods and to reuse StorageObject instead of another custom storage object constructor.

CI: https://ci.nodejs.org/job/node-test-pull-request/6518/

Contributor

mscdex commented Feb 20, 2017

Updated PR to use one of the new http API methods and to reuse StorageObject instead of another custom storage object constructor.

CI: https://ci.nodejs.org/job/node-test-pull-request/6518/

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Feb 21, 2017

Member

do we have any ecosystem usage metrics for this? that's my only concern before agreeing.

Member

rvagg commented Feb 21, 2017

do we have any ecosystem usage metrics for this? that's my only concern before agreeing.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 21, 2017

Contributor

@rvagg I don't know, all I can do is run in CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/586/

express/send has already started the process of migrating to using the existing getHeader() instead of _headers FWIW.

Contributor

mscdex commented Feb 21, 2017

@rvagg I don't know, all I can do is run in CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/586/

express/send has already started the process of migrating to using the existing getHeader() instead of _headers FWIW.

@rvagg

This comment has been minimized.

Show comment
Hide comment
@rvagg

rvagg Feb 21, 2017

Member

@ChALkeR are you still the one to find this data?

Member

rvagg commented Feb 21, 2017

@ChALkeR are you still the one to find this data?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Feb 22, 2017

Member

P.S. I know I shouldn't have done this, but had some code that was

function clearHeaders (res) {
  res._headers = {}
  res._headerNames = {}
}

Obviously needs to change. Right now works on Node.js 8, but this landing will break even that without any warning. Should probably keep that working with a warning if the getters would keep working, right?

Member

dougwilson commented Feb 22, 2017

P.S. I know I shouldn't have done this, but had some code that was

function clearHeaders (res) {
  res._headers = {}
  res._headerNames = {}
}

Obviously needs to change. Right now works on Node.js 8, but this landing will break even that without any warning. Should probably keep that working with a warning if the getters would keep working, right?

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 22, 2017

Contributor

@dougwilson Yeah I'm not sure what the best solution to that would be as far as the behavior of the setter, since we'd probably have to just copy the values out of the object instead of just using the same object reference (because the internal representation changed). Should we blindly copy values in the ._headers setter or should we pass each name and value to .setHeader()?

Thoughts on this @nodejs/collaborators ?

Contributor

mscdex commented Feb 22, 2017

@dougwilson Yeah I'm not sure what the best solution to that would be as far as the behavior of the setter, since we'd probably have to just copy the values out of the object instead of just using the same object reference (because the internal representation changed). Should we blindly copy values in the ._headers setter or should we pass each name and value to .setHeader()?

Thoughts on this @nodejs/collaborators ?

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Feb 22, 2017

Member

I've only seen it used as = {} or = null. It would be interesting if anything out there is actually doing anything more complex, but at least for this (only thing I've come across personally) just throwing on set maybe the best option (or warning + do nothing), otherwise I'd get too complex, probably. If it wasn't silent that would probably help people track it down, at least :shurgs:

Member

dougwilson commented Feb 22, 2017

I've only seen it used as = {} or = null. It would be interesting if anything out there is actually doing anything more complex, but at least for this (only thing I've come across personally) just throwing on set maybe the best option (or warning + do nothing), otherwise I'd get too complex, probably. If it wasn't silent that would probably help people track it down, at least :shurgs:

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 22, 2017

Member

Throwing in the setter would likely be best but would violate our general approach around deprecating. If we are not able to come up with any reasonably obvious use cases where something more complex than = {} and = null are used, then I think a straightforward copy of values from along with the deprecation warning would be sufficient. Along with that, we need to very clearly articulate that the behavior is strongly discouraged and will quickly (by Node.js 9.0.0) be escalated into a throw.

Member

jasnell commented Feb 22, 2017

Throwing in the setter would likely be best but would violate our general approach around deprecating. If we are not able to come up with any reasonably obvious use cases where something more complex than = {} and = null are used, then I think a straightforward copy of values from along with the deprecation warning would be sufficient. Along with that, we need to very clearly articulate that the behavior is strongly discouraged and will quickly (by Node.js 9.0.0) be escalated into a throw.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Feb 22, 2017

Member

@rvagg, I will check this today.

Upd: rebuild in progress =).

Member

ChALkeR commented Feb 22, 2017

@rvagg, I will check this today.

Upd: rebuild in progress =).

@mcollina

LGTM

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 23, 2017

Contributor

Alright, I've added setters for the deprecated properties now and updated the new test file.

The ._headers setter is straight forward, it currently just copies values as-is, without validation.

The ._headerNames setter is a little tricky/awkward because of the way the information is now stored internally, so what the setter currently does is it only updates the original header names specified in the passed object for matching header names in the combined internal object (this[outHeadersKey]).

Contributor

mscdex commented Feb 23, 2017

Alright, I've added setters for the deprecated properties now and updated the new test file.

The ._headers setter is straight forward, it currently just copies values as-is, without validation.

The ._headerNames setter is a little tricky/awkward because of the way the information is now stored internally, so what the setter currently does is it only updates the original header names specified in the passed object for matching header names in the combined internal object (this[outHeadersKey]).

Show outdated Hide outdated lib/_http_outgoing.js
set: internalUtil.deprecate(function(val) {
if (val == null) {
this[outHeadersKey] = null;
} else if (typeof val === 'object') {

This comment has been minimized.

@jasnell

jasnell Feb 23, 2017

Member

hmm... if the val happens to be an Array this would have some odd effects. I don't want this getting any more complicated so perhaps that's ok, but I'd rather not see header name keys like 0: whatever, 1: value, etc

@jasnell

jasnell Feb 23, 2017

Member

hmm... if the val happens to be an Array this would have some odd effects. I don't want this getting any more complicated so perhaps that's ok, but I'd rather not see header name keys like 0: whatever, 1: value, etc

This comment has been minimized.

@mscdex

mscdex Feb 23, 2017

Contributor

What odd effects? Prior to all of these http changes, unless there were numeric headers set in ._headers, there would never be a match when node would do ._headerNames[lowercasedName] (which would have already been causing issues on its own for quite awhile now I would assume) to find the original-cased name when rendering the headers.

@mscdex

mscdex Feb 23, 2017

Contributor

What odd effects? Prior to all of these http changes, unless there were numeric headers set in ._headers, there would never be a match when node would do ._headerNames[lowercasedName] (which would have already been causing issues on its own for quite awhile now I would assume) to find the original-cased name when rendering the headers.

This comment has been minimized.

@jasnell

jasnell Feb 23, 2017

Member

True, sigh. Ok, I guess just leave it as is then until I can think of a better idea.

@jasnell

jasnell Feb 23, 2017

Member

True, sigh. Ok, I guess just leave it as is then until I can think of a better idea.

This comment has been minimized.

@mscdex

mscdex Feb 26, 2017

Contributor

until I can think of a better idea

Before or after this PR gets landed?

@mscdex

mscdex Feb 26, 2017

Contributor

until I can think of a better idea

Before or after this PR gets landed?

This comment has been minimized.

@jasnell

jasnell Feb 26, 2017

Member

After. Go for it.

@jasnell

jasnell Feb 26, 2017

Member

After. Go for it.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 26, 2017

Contributor

@nodejs/ctc Is this good to land? The sooner we can land this, the sooner some of the errors in CITGM can go away and nightlies will become less breaking...

Contributor

mscdex commented Feb 26, 2017

@nodejs/ctc Is this good to land? The sooner we can land this, the sooner some of the errors in CITGM can go away and nightlies will become less breaking...

@dougwilson

This comment has been minimized.

Show comment
Hide comment
@dougwilson

dougwilson Feb 26, 2017

Member

I got express and others setup in our Travis CI to test against the 8.0 nightly builds and we're passing these days. From my understanding CITGM won't pick up until our next release, which will be in a few days and the express failures on master should be gone. Can't really speak for the spdy failures, though, though it sounds like this would resolve that.

Member

dougwilson commented Feb 26, 2017

I got express and others setup in our Travis CI to test against the 8.0 nightly builds and we're passing these days. From my understanding CITGM won't pick up until our next release, which will be in a few days and the express failures on master should be gone. Can't really speak for the spdy failures, though, though it sounds like this would resolve that.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Feb 26, 2017

Member

@rvagg I have rebuilt the dataset, sorry for the delay.

To my understanding, the ecosystem usage on both _headerNames and _headers is still pretty high, so I'm not sure about a runtime warning on those yet…

I will post some data shortly.

Member

ChALkeR commented Feb 26, 2017

@rvagg I have rebuilt the dataset, sorry for the delay.

To my understanding, the ecosystem usage on both _headerNames and _headers is still pretty high, so I'm not sure about a runtime warning on those yet…

I will post some data shortly.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Feb 26, 2017

Member

\._headerNames: https://gist.github.com/ChALkeR/aed3c5ed8c9d3e8348a5076947f743d8
\._headers[^a-zA-Z]: https://gist.github.com/ChALkeR/c75aeb16d24f8994fdcf1bc5637b5742

Note: there is probably a significant amount of false positives due to feature detection in the matched modules.

A reminder: that's just the current versions of all modules, it might mismatch with the version that is actively used in fact, and it doesn't take deps (and their versions) into an account.

Perhaps we should start with a docs-only deprecation first?

Member

ChALkeR commented Feb 26, 2017

\._headerNames: https://gist.github.com/ChALkeR/aed3c5ed8c9d3e8348a5076947f743d8
\._headers[^a-zA-Z]: https://gist.github.com/ChALkeR/c75aeb16d24f8994fdcf1bc5637b5742

Note: there is probably a significant amount of false positives due to feature detection in the matched modules.

A reminder: that's just the current versions of all modules, it might mismatch with the version that is actively used in fact, and it doesn't take deps (and their versions) into an account.

Perhaps we should start with a docs-only deprecation first?

@ChALkeR

This should be at least mentioned on the docs, like #11355 did, despite it being a never documented api. The usage was pretty big.

Also I'm unsure about runtime-deprecating this in 8.0, given that the usage is still pretty high and that the express/send versions that avoid hitting that code were just released. Perhaps we need to discuss that a bit more. What are the immediate benefits from that in 8.x, again?

@jasnell

@ChALkeR makes a great point. This needs documentation in deprecations.md and a deprecation code assigned on the internalUtil.deprecate() calls.

@mscdex

This comment has been minimized.

Show comment
Hide comment
Contributor

mscdex commented Feb 27, 2017

Show outdated Hide outdated doc/api/deprecations.md
@@ -534,6 +534,21 @@ deprecated. Please use `ServerResponse.prototype.writeHead()` instead.
*Note*: The `ServerResponse.prototype.writeHeader()` method was never documented
as an officially supported API.
<a id="DEP0064"></a>
#### DEP0064: outgoingMessage._headers, outgoingMessage._headerNames

This comment has been minimized.

@ChALkeR

ChALkeR Feb 27, 2017

Member

Needs backslashes here.

@ChALkeR

ChALkeR Feb 27, 2017

Member

Needs backslashes here.

This comment has been minimized.

@mscdex

mscdex Feb 27, 2017

Contributor

Added.

@mscdex

mscdex Feb 27, 2017

Contributor

Added.

Show outdated Hide outdated doc/api/deprecations.md
@@ -534,6 +534,21 @@ deprecated. Please use `ServerResponse.prototype.writeHead()` instead.
*Note*: The `ServerResponse.prototype.writeHeader()` method was never documented
as an officially supported API.
<a id="DEP0064"></a>
### DEP0064: outgoingMessage.\_headers, outgoingMessage.\_headerNames

This comment has been minimized.

@jasnell

jasnell Feb 27, 2017

Member

Note: depending on when this lands, the code may need to be updated (e.g. if any other deprecations land first) ... I don't suspect that will be an issue, but just to be aware

@jasnell

jasnell Feb 27, 2017

Member

Note: depending on when this lands, the code may need to be updated (e.g. if any other deprecations land first) ... I don't suspect that will be an issue, but just to be aware

This comment has been minimized.

@mscdex

mscdex Feb 27, 2017

Contributor

If someone tries to land a deprecation with this number before I do, the matter will be settled in an office joust.

@mscdex

mscdex Feb 27, 2017

Contributor

If someone tries to land a deprecation with this number before I do, the matter will be settled in an office joust.

This comment has been minimized.

Updated to a doc-deprecation

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Feb 28, 2017

Contributor

I've updated the PR to add another deprecation for an undocumented method (_renderHeaders()).

Contributor

mscdex commented Feb 28, 2017

I've updated the PR to add another deprecation for an undocumented method (_renderHeaders()).

Show outdated Hide outdated doc/api/deprecations.md
Type: Documentation-only
The `http` module `OutgoingMessage.prototype._headers` has been deprecated.

This comment has been minimized.

@dougwilson

dougwilson Feb 28, 2017

Member

Did this line mean to say _renderHeaders?

@dougwilson

dougwilson Feb 28, 2017

Member

Did this line mean to say _renderHeaders?

This comment has been minimized.

@mscdex

mscdex Feb 28, 2017

Contributor

Fixed.

@mscdex

mscdex Feb 28, 2017

Contributor

Fixed.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 3, 2017

Contributor

Any further comments/suggestions/etc. before landing, @nodejs/collaborators?

Contributor

mscdex commented Mar 3, 2017

Any further comments/suggestions/etc. before landing, @nodejs/collaborators?

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Mar 3, 2017

Member

@mscdex Are you aware of any usage pattern that would be broken with the getters/setters approach?
I.e. is the second commit (http: reuse existing StorageObject) semver-major?

Member

ChALkeR commented Mar 3, 2017

@mscdex Are you aware of any usage pattern that would be broken with the getters/setters approach?
I.e. is the second commit (http: reuse existing StorageObject) semver-major?

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 3, 2017

Contributor

@ChALkeR It seems like that's two separate questions?

I don't know offhand of situations where the getters/setters introduced would cause problems. AFAIK most use cases it should work fine, just like any other place where we've used getters/setters (especially for deprecation purposes).

Regarding the second commit, that's just reducing unnecessary code. Not only is StorageObject/OutgoingHeaders not easily exposed to userland, but I doubt anyone would be doing something like instanceof OutgoingHeaders, especially since it hasn't been in core for that long.

Contributor

mscdex commented Mar 3, 2017

@ChALkeR It seems like that's two separate questions?

I don't know offhand of situations where the getters/setters introduced would cause problems. AFAIK most use cases it should work fine, just like any other place where we've used getters/setters (especially for deprecation purposes).

Regarding the second commit, that's just reducing unnecessary code. Not only is StorageObject/OutgoingHeaders not easily exposed to userland, but I doubt anyone would be doing something like instanceof OutgoingHeaders, especially since it hasn't been in core for that long.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Mar 5, 2017

Member

@mscdex

It seems like that's two separate questions?

Yes, sorry, I missed something here, you are correct =).

Then, why does the first commit have both the docs deprecation and the switch to getters/setters? Shouldn't those be separate? Btw, that commit has no detailed message and does not mention getters/setters.

Member

ChALkeR commented Mar 5, 2017

@mscdex

It seems like that's two separate questions?

Yes, sorry, I missed something here, you are correct =).

Then, why does the first commit have both the docs deprecation and the switch to getters/setters? Shouldn't those be separate? Btw, that commit has no detailed message and does not mention getters/setters.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 5, 2017

Contributor

The switch to getters/setters is made because we're already using the private properties differently. I don't care one way or the other if they're separate commits. Originally it was a runtime deprecation, so the getters/setters printed a deprecation message.

Contributor

mscdex commented Mar 5, 2017

The switch to getters/setters is made because we're already using the private properties differently. I don't care one way or the other if they're separate commits. Originally it was a runtime deprecation, so the getters/setters printed a deprecation message.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 6, 2017

Contributor

@ChALkeR I've split the commit now.

Contributor

mscdex commented Mar 6, 2017

@ChALkeR I've split the commit now.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 6, 2017

Contributor

I'll land this on Tuesday if there are no more objections before then.

Here's another CI: https://ci.nodejs.org/job/node-test-pull-request/6720/

Contributor

mscdex commented Mar 6, 2017

I'll land this on Tuesday if there are no more objections before then.

Here's another CI: https://ci.nodejs.org/job/node-test-pull-request/6720/

@mikeal

This comment has been minimized.

Show comment
Hide comment
@mikeal

mikeal Mar 6, 2017

Member

It looks like a lot of reverse compatibility work has been done here, can we get a redux of what won't work anymore after this change?

Member

mikeal commented Mar 6, 2017

It looks like a lot of reverse compatibility work has been done here, can we get a redux of what won't work anymore after this change?

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Mar 7, 2017

Member

I'm still a bit concerned with deprecating res._headers. Although it isn't documented as a public api, it is used in so many places. Is it really justifiable to break the ecosystem like this?

Member

evanlucas commented Mar 7, 2017

I'm still a bit concerned with deprecating res._headers. Although it isn't documented as a public api, it is used in so many places. Is it really justifiable to break the ecosystem like this?

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 7, 2017

Contributor

@mikeal As far as I know there shouldn't really be any difference with the getters/setters in place.

@evanlucas I don't understand the concern, there is no runtime deprecation anymore. It's a documentation deprecation. Also there are now other public http APIs that cover all of the use cases I've seen thus far for accessing the private properties.

Contributor

mscdex commented Mar 7, 2017

@mikeal As far as I know there shouldn't really be any difference with the getters/setters in place.

@evanlucas I don't understand the concern, there is no runtime deprecation anymore. It's a documentation deprecation. Also there are now other public http APIs that cover all of the use cases I've seen thus far for accessing the private properties.

@evanlucas

This comment has been minimized.

Show comment
Hide comment
@evanlucas

evanlucas Mar 8, 2017

Member

@mscdex even with a docs only deprecation, that still more than likely means we are going to remove it eventually. I'm just not sure whether it is justified.

I do seem to be in the minority on this one though so ¯\(ツ)

Member

evanlucas commented Mar 8, 2017

@mscdex even with a docs only deprecation, that still more than likely means we are going to remove it eventually. I'm just not sure whether it is justified.

I do seem to be in the minority on this one though so ¯\(ツ)

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Mar 8, 2017

Contributor

@evanlucas Well yes, that is the plan for deprecations. I don't see how it's a problem considering it's only a docs deprecation for v8.0.0 and there are public methods that cover the current use cases for these private properties (these methods should be backported to v4.x and v6.x). IMHO there will be plenty of time for people to convert their code to use these new methods before the old, private properties become a runtime deprecation and eventually removed.

This PR also fixes a backwards compatibility issue that currently exists in master, so that is why I've been wanting to get this landed sooner than later.

Contributor

mscdex commented Mar 8, 2017

@evanlucas Well yes, that is the plan for deprecations. I don't see how it's a problem considering it's only a docs deprecation for v8.0.0 and there are public methods that cover the current use cases for these private properties (these methods should be backported to v4.x and v6.x). IMHO there will be plenty of time for people to convert their code to use these new methods before the old, private properties become a runtime deprecation and eventually removed.

This PR also fixes a backwards compatibility issue that currently exists in master, so that is why I've been wanting to get this landed sooner than later.

@mscdex

This comment has been minimized.

Show comment
Hide comment

mscdex added some commits Mar 6, 2017

doc: deprecate private http properties
PR-URL: #10941
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
http: use Symbol for outgoing headers
PR-URL: #10941
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
http: reuse existing StorageObject
PR-URL: #10941
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
http: support old private properties and function
PR-URL: #10941
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

@mscdex mscdex changed the title from http: add deprecation warnings for private props to http: add doc deprecation for private props Mar 9, 2017

@mscdex mscdex merged commit 8243ca0 into nodejs:master Mar 9, 2017

@mscdex mscdex removed the ctc-review label Mar 9, 2017

@mscdex mscdex deleted the mscdex:http-outgoingmessage-headers-deprecate branch Mar 9, 2017

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017

doc: deprecate private http properties
PR-URL: nodejs#10941
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017

http: use Symbol for outgoing headers
PR-URL: nodejs#10941
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017

http: support old private properties and function
PR-URL: nodejs#10941
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

jungx098 added a commit to jungx098/node that referenced this pull request Mar 21, 2017

http: reuse existing StorageObject
PR-URL: nodejs#10941
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

@jasnell jasnell referenced this pull request Apr 4, 2017

Closed

8.0.0 Release Proposal #12220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment