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: revert deprecation of client property #1852

Closed
wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented May 31, 2015

Reason: breaks a feature in the request module

See #1850
Replaces #1851

/cc @mscdex

@@ -47,7 +47,7 @@ function IncomingMessage(socket) {
// response (client) only
this.statusCode = null;
this.statusMessage = null;
this._client = socket; // deprecated
this.client = socket;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we define client in this instead of IncomingMessage.prototype and issue the deprecation warning as it is in the old code?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not against it

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, but then we will be creating many functions, whenever we create a new object. It might impact the performance. Not sure if it is okay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wouldn't something like this work ?

// define it once somewhere
var clientProperty = {      
  configurable: true,       
  enumerable: true,     
  get: util.deprecate(function() {      
    return this._client;        
  }, 'client is deprecated, use socket or connection instead'),     
  set: util.deprecate(function(val) {       
    this._client = val;     
  }, 'client is deprecated, use socket or connection instead')      
});

// IncomingMessage constructor
Object.defineProperty(this, 'client', clientProperty)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ya. That should work. Are you going to change it or wait for TSC guys?

Edit: I think you have an extra paren at the end.

Copy link
Member

Choose a reason for hiding this comment

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

A quick question: why suggest using «socket or connection» instead of just one of those? «Use socket instead» would be more straightforward.

.connection is from the first version of the http module, .socket got there when the http module was replaced with the second version. Then .connection got back in for compatibility. Both of those go as far as node 0.1.90 (node 0.2.0).

There is no actual need for keeping both of them, except for backwards compatibility. If one of those would be deprecated in further versions, the current message would add more inconvenience to the migration process:
— «client is deprecated, use socket or connection instead»
— ok, connection
— «connection is deprecated, use socket instead»
— …

Why not choose one of them now and recommend it? socket is used internally atm.

I do not agitate for immediate .connection deprecation, I just wanted to say that altering the message a bit would make further deprecation of one of those in some later version (if that happens) more convenient. Also it would not raise an extra question «should I use socket or connection here instead of client?».

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say that socket and connection are used far more than client, so I would hold off on making deprecation messages for them.

Regarding Object.defineProperty() in the constructor, did you try this change with the request module to make sure it is ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mscdex Would make test-npm cover testing with request module?

Copy link
Member

Choose a reason for hiding this comment

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

@mscdex
Your comment looks like you thought that I suggested deprecating one of those.
I just suggested to change the deprecation message for .client to mention just one of those.

@targos
Copy link
Member Author

targos commented May 31, 2015

moved the Object.defineProperty call inside the constructor. Deprecation message is now "client is deprecated, use socket instead".

@targos
Copy link
Member Author

targos commented May 31, 2015

make test passes, running make test-npm

@targos
Copy link
Member Author

targos commented May 31, 2015

make test-npm fails, but it seems to be unrelated to the ssl issue (first time I am running this task):

2> npm ERR! tar pack Error reading /Users/mzasso/git/targos/io.js/test-npm
 2> npm ERR! addLocalDirectory Could not pack /Users/mzasso/git/targos/io.js/test-npm to /var/folders/4x/ffqx483s2wddw7mlzt10cgmm0000gp/T/npm-test-77672/npm_cache/npm/2.11.0/package.tgz
 2> npm ERR! addLocal Could not install /Users/mzasso/git/targos/io.js/test-npm
 2> npm ERR! Darwin 14.3.0
 2> npm ERR! argv "/usr/local/bin/iojs" "/Users/mzasso/git/targos/io.js/test-npm/bin/npm-cli.js" "install" "/Users/mzasso/git/targos/io.js/test-npm"
 2> npm ERR! node v2.1.0
 2> npm ERR! npm  v2.11.0
 2> npm ERR! path /Users/mzasso/git/targos/io.js/test-npm/node_modules/inherits/README.md
 2> npm ERR! code ENFILE
 2> npm ERR! errno -23
 2> npm ERR! syscall open
 2> 
 2> npm ERR! ENFILE: file table overflow, open '/Users/mzasso/git/targos/io.js/test-npm/node_modules/inherits/README.md'
 2> npm ERR! 
 2> npm ERR! If you need help, you may report this error at:
 2> npm ERR!     <https://github.com/npm/npm/issues>
 2> 
 2> npm ERR! Please include the following file with any support request:
 2> npm ERR!     /private/var/folders/4x/ffqx483s2wddw7mlzt10cgmm0000gp/T/npm-test-77672/root/npm-debug.log
 2> 
not ok 1 node "/Users/mzasso/git/targos/io.js/test-npm/bin/npm-cli.js" install "/Users/mzasso/git/targos/io.js/test-npm"
1..1
/Users/mzasso/git/targos/io.js/test-npm/test/run.js:192
  if (er) throw er
                ^
Error: failed node "/Users/mzasso/git/targos/io.js/test-npm/bin/npm-cli.js" install "/Users/mzasso/git/targos/io.js/test-npm"
    at /Users/mzasso/git/targos/io.js/test-npm/test/run.js:104:10
    at ChildProcess.exithandler (child_process.js:692:5)
    at emitTwo (events.js:87:13)
    at ChildProcess.emit (events.js:172:7)
    at maybeClose (child_process.js:953:16)
    at Socket.<anonymous> (child_process.js:1114:11)
    at emitOne (events.js:77:13)
    at Socket.emit (events.js:169:7)
    at Pipe._onclose (net.js:472:12)

npm ERR! Darwin 14.3.0
npm ERR! argv "/usr/local/bin/iojs" "/usr/local/bin/npm" "run" "test-legacy"
npm ERR! node v2.1.0
npm ERR! npm  v2.10.1
npm ERR! code ELIFECYCLE
npm ERR! [email protected] test-legacy: `node ./test/run.js`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] test-legacy script 'node ./test/run.js'.
npm ERR! This is most likely a problem with the npm package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     node ./test/run.js
npm ERR! You can get their info via:
npm ERR!     npm owner ls npm
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/mzasso/git/targos/io.js/test-npm/npm-debug.log

npm ERR! Darwin 14.3.0
npm ERR! argv "/Users/mzasso/git/targos/io.js/out/Release/iojs" "/Users/mzasso/git/targos/io.js/test-npm/cli.js" "run-script" "test-all"
npm ERR! node v2.2.1
npm ERR! npm  v2.11.0
npm ERR! code ELIFECYCLE
npm ERR! [email protected] test-all: `npm run test-legacy && npm test`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the [email protected] test-all script 'npm run test-legacy && npm test'.
npm ERR! This is most likely a problem with the npm package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     npm run test-legacy && npm test
npm ERR! You can get their info via:
npm ERR!     npm owner ls npm
npm ERR! There is likely additional logging output above.

npm ERR! Please include the following file with any support request:
npm ERR!     /Users/mzasso/git/targos/io.js/test-npm/npm-debug.log
make: *** [test-npm] Error 1

@Fishrock123
Copy link
Contributor

@targos See #1850 (comment) -- it appears there is a 2nd error.

Edit: wait that looks different, can you try it again?

@ChALkeR
Copy link
Member

ChALkeR commented May 31, 2015

The commit message is wrong atm.
The commit doesn't revert the deprecation after changes, it fixes it.

@silverwind
Copy link
Contributor

LGTM regarding fixing the issue. I can't get test-npm to complete (https://gist.github.com/silverwind/a94589df8e3aae1907e1), but at least the SSL error is gone with this and the deprecation prints correctly.

@silverwind
Copy link
Contributor

Suggesting http: fix deprecation of client property as the commit message.

enumerable: true,
get: util.deprecate(function() {
return this.socket;
}, 'http.IncomingRequest.client is deprecated, use .socket instead'),
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be http.IncomingMessage?

Copy link
Member Author

Choose a reason for hiding this comment

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

you are right, updated

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, thanks for catching that.

@targos targos force-pushed the revert-client branch 2 times, most recently from d3f52bb to f2ad68f Compare May 31, 2015 17:04
@targos targos changed the title http: revert deprecation of client property http: fix deprecation of client property May 31, 2015
@mscdex mscdex added the http Issues or PRs related to the http subsystem. label May 31, 2015
@Fishrock123
Copy link
Contributor

LGTM. If people would rather just revert this deprecation altogether that also LGTM.

@bnoordhuis
Copy link
Member

Let's just revert the deprecation warning. We know now that people use the property so let's keep it around for now.

Reason: breaks a feature in the request module used by the bundled npm
@targos targos changed the title http: fix deprecation of client property http: revert deprecation of client property May 31, 2015
@othiym23
Copy link
Contributor

We know now that people use the property so let's keep it around for now.

I think it would be a good idea to recognize (or even make policy) that the determining factor for whether an object property on something created by Node is "used" shouldn't be whether that property is documented or used within the Node code base itself. Node has been around long enough that it shouldn't be a surprise to people that the ecosystem may be relying on attributes or behavior that Node itself isn't.

@targos
Copy link
Member Author

targos commented May 31, 2015

moved again to a revert of the deprecation

@bnoordhuis
Copy link
Member

LGTM. Next one to LGTM should land it. (See what I did there?)

CI: https://jenkins-iojs.nodesource.com/view/iojs/job/iojs+any-pr+multi/738/

@thefourtheye
Copy link
Contributor

@bnoordhuis LGTM, and I cannot land it :D

@silverwind
Copy link
Contributor

If it matters, it was this.socket instead of socket before:

1eec5f0#diff-f7d8a311cc7ac30522e414465bda2229L52

@ChALkeR
Copy link
Member

ChALkeR commented May 31, 2015

@silverwind No, it shouldn't matter.

@silverwind
Copy link
Contributor

Okay, landing

silverwind pushed a commit that referenced this pull request May 31, 2015
The improper deprecation of the property broke a feature in the
request module used by the bundled npm. This reverts the deprecation
part of this change.

PR-URL: #1852
Fixes: #1850
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
@silverwind
Copy link
Contributor

Landed in 4d6b768

@silverwind silverwind closed this May 31, 2015
@ChALkeR
Copy link
Member

ChALkeR commented May 31, 2015

Yay. Could we have a 2.2.1 now?

@Fishrock123
Copy link
Contributor

I think it would be a good idea to recognize (or even make policy) that the determining factor for whether an object property on something created by Node is "used" shouldn't be whether that property is documented or used within the Node code base itself. Node has been around long enough that it shouldn't be a surprise to people that the ecosystem may be relying on attributes or behavior that Node itself isn't.

@othiym23 Yeah, but only a couple of us have access to an npm clone to statically analyze with @chrisdickinson 's estoc (myself not included). I've made an issue over on that repo for issues that we need static analysis for, but unfortunately this landed without it. chrisdickinson/estoc#1

rvagg added a commit to rvagg/io.js that referenced this pull request Jun 1, 2015
Notable Changes:

* http: reverts the removal of an undocumented `client` property on client
  connections, this property is being used in the wild, most notably by
  https://github.com/request/request which is used by npm.
  (Michaël Zasso) [nodejs#1852](nodejs#1852).
@rvagg rvagg mentioned this pull request Jun 1, 2015
rvagg added a commit to rvagg/io.js that referenced this pull request Jun 1, 2015
PR-URL: nodejs#1856

Notable Changes:

* http: reverts the removal of an undocumented `client` property on client
  connections, this property is being used in the wild, most notably by
  https://github.com/request/request which is used by npm.
  (Michaël Zasso) [nodejs#1852](nodejs#1852).
rvagg added a commit to rvagg/io.js that referenced this pull request Jun 1, 2015
PR-URL: nodejs#1856

Notable Changes:

* http: reverts the removal of an undocumented `client` property on client
  connections, this property is being used in the wild, most notably by
  https://github.com/request/request which is used by npm.
  (Michaël Zasso) [nodejs#1852](nodejs#1852).
@targos targos deleted the revert-client branch June 1, 2015 04:01
@othiym23
Copy link
Contributor

othiym23 commented Jun 1, 2015

@Fishrock123 I didn't mean that we should assay the corpus or use fancy static analysis tools, although those are other potential solutions. That is always going to be at least partially heuristic. I meant that the project as a whole should be conservative about removing or deprecating things, because we can't know what's being relied upon by third parties. Node is a semi-mature product, and should act like it.

andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this pull request Jun 3, 2015
The improper deprecation of the property broke a feature in the
request module used by the bundled npm. This reverts the deprecation
part of this change.

PR-URL: nodejs/node#1852
Fixes: nodejs/node#1850
Reviewed-By: Jeremiah Senkpiel <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants