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

Closed
wants to merge 1 commit into from
Closed

http: revert deprecation of client property #1852

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions lib/_http_incoming.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.


// flag for backwards compatibility grossness.
this._consuming = false;
Expand All @@ -61,16 +61,6 @@ util.inherits(IncomingMessage, Stream.Readable);

exports.IncomingMessage = IncomingMessage;

Object.defineProperty(IncomingMessage.prototype, 'client', {
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.prototype.setTimeout = function(msecs, callback) {
if (callback)
Expand Down