util: allow deprecate on classes #7690

vdeturckheim · 2016-07-12T21:11:48Z

Checklist

tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

util

Description of change

Classes objects cannot be called without new. util.deprecate executes 'fn.apply'. Classes cannot be deprecated with util.deprecate.

This uses 'new.target' to detect (when process is defined) calls to constructor to allow deprecate on class.

cjihrig · 2016-07-13T01:29:06Z

test/fixtures/deprecated-userland-class.js

+class deprecatedClass {
+}
+
+var depreciated = util.deprecate(deprecatedClass, 'deprecatedClass is deprecated.');


Should this be deprecated? Also, const please.

Ack. Sorry, French-English mixin in my brain

vdeturckheim · 2016-07-13T08:05:10Z

@cjihrig Thanks a lot for the review !
I think I have fixed what you pointed. Do you think I should also remove console statements and enforce strictEqual for the other tests (already present) in test/sequential/test-deprecation-flags.js ?

cjihrig · 2016-07-13T12:36:22Z

@vdeturckheim no need to change the existing test. Thanks though.

targos · 2016-07-13T13:08:31Z

lib/internal/util.js

@@ -60,6 +60,9 @@ exports._deprecate = function(fn, msg) {
 var warned = false;
 function deprecated() {
 warned = exports.printDeprecationMessage(msg, warned, deprecated);
+ if (new.target) {
+ return new (Function.prototype.bind.apply(fn, arguments));


I think it's important to make the deprecated class correctly subclassable. Can you change this line to return Reflect.construct(fn, arguments, new.target) and add a test that verifies that ?

Ack.
Seems fair to me.

I think a test should be added with a subclass as well to make confirm the functionality

Fishrock123 · 2016-07-13T13:09:17Z

Seems reasonable to me, I bet core will need it at some point in the future.

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

cjihrig · 2016-07-14T12:51:54Z

test/fixtures/deprecated-userland-subclass.js

+const deprecated = util.deprecate(deprecatedClass, 'deprecatedClass is deprecated.');
+
+class subclass extends deprecated {
+


No need for this blank line.

cjihrig · 2016-07-14T12:52:42Z

Can you remove all of the additional commits from this PR?

cjihrig · 2016-07-14T13:16:08Z

doc/api/util.md

@@ -46,7 +46,7 @@ environment variable. For example: `NODE_DEBUG=fs,net,tls`.

 ## util.deprecate(function, string)

-The `util.deprecate()` method wraps the given `function` in such a way that
+The `util.deprecate()` method wraps the given `function` or `class` in such a way that


I think the backticks around "function" here, refer to the named parameter. If that's the case, I'm not sure it makes sense to put "class" in backticks too.

cjihrig · 2016-07-14T13:16:17Z

One small comment, but LGTM.

targos · 2016-07-14T13:39:22Z

Looking at it again, I am not sure it's doing what we expect it to.

Example:

class A {
  hello() {
    console.log('world');
  }
}

const DA = util.deprecate(A, 'A is deprecated');

var da = new DA(); // logs deprecation message
da.hello(); // throws

To fix this, we need to pass B as a third argument to Reflect.construct. But then comes the problem of inheritance.

targos · 2016-07-14T13:42:34Z

Maybe we should change the prototype chain of the returned function to include A.prototype ?

Something like:

Object.setPrototypeOf(deprecated, A);
Object.setPrototypeOf(deprecated.prototype, A.prototype);

vdeturckheim · 2016-07-14T14:14:01Z

@targos your solution seems to work:

const deprecate = function(fn, msg) {

    var warned = false;
    function deprecated() {
        warned = console.log(msg);
        if (new.target) {
            return Reflect.construct(fn, arguments, new.target);
        }
        return fn.apply(this, arguments);
    }

    Object.setPrototypeOf(deprecated, fn);
    Object.setPrototypeOf(deprecated.prototype, fn.prototype);

    return deprecated;
};

class A{};
const DA = deprecate(A, 'A is deprecated');

class B extends DA{
    constructor() {
        super();
    }
}

const b = new B();

console.log(b instanceof B); // true
console.log(b instanceof A); // true
console.log(b instanceof DA); // true

targos · 2016-07-14T14:31:55Z

test/fixtures/deprecated-userland-class.js

+
+const deprecated = util.deprecate(deprecatedClass, 'deprecatedClass is deprecated.');
+
+new deprecated();


can you add a few instanceof checks ?

targos · 2016-07-14T15:15:58Z

LGTM

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

Classes objects cannot be called without new. util.deprecate executes 'fn.apply'. Therefore, classes cannot be deprecated with util.deprecate. This uses 'new.target' to detect (when process is defined) calls to constructor to allow deprecate on class.

vdeturckheim · 2016-07-14T16:06:18Z

I added a check on the presence of fn.prototype before assignation

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

targos · 2016-07-14T18:08:41Z

Oh yes, I forgot about arrow functions.

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

vdeturckheim · 2016-07-19T13:12:32Z

@targos Is there anything else I need to do before merge ? :)

targos · 2016-07-19T13:38:26Z

@vdeturckheim No, everything's fine. Waiting for other collaborators to review :)

jasnell · 2016-07-29T17:01:53Z

test/sequential/test-deprecation-flags.js

@@ -39,10 +45,22 @@ execFile(node, traceDep, function(er, stdout, stderr) {
 console.log('trace ok');
 });

-execFile(node, [depUserland], function(er, stdout, stderr) {
+execFile(node, [depUserlandFunction], function(er, stdout, stderr) {
 console.error('normal: testing deprecated userland function');
 assert.equal(er, null);
 assert.equal(stdout, '');
 assert(/deprecatedFunction is deprecated/.test(stderr));


FWIW, an easier way to test for the deprecation warning would be to listen for warning events...

process.on('warning', (warning) => { if (warning.name === 'DeprecationWarning') { // ... } });

But that doesn't need to be fixed here at all.. just pointing it out

jasnell · 2016-07-29T17:15:32Z

LGTM. Good stuff.

jasnell · 2016-08-01T18:42:22Z

It's been a while since the last CI run: https://ci.nodejs.org/job/node-test-pull-request/3489/

vdeturckheim · 2016-08-02T08:52:24Z

@jasnell I don't really understand why the build fails now :/

addaleax · 2016-08-02T09:46:55Z

@vdeturckheim That’s an unrelated build problem, nothing to worry about.

New CI attempt: https://ci.nodejs.org/job/node-test-commit/4369/

cjihrig · 2016-08-02T13:33:48Z

Looks like more unrelated failures.

vdeturckheim · 2016-08-02T13:37:50Z

:( Is there anything I can/should do here ?

addaleax · 2016-08-02T13:46:13Z

Probably no… the last two CI runs together are green, though, and one of the flaky tests from the last run is just being fixed.

vdeturckheim · 2016-08-02T13:56:53Z

Ok, thanks :)

cjihrig · 2016-08-04T15:56:03Z

Landed in 320f433. Thanks, and sorry for the delay.

Classes cannot be instantiated without new, but util.deprecate() uses Function.prototype.apply(). This commit uses new.target to detect constructor calls, allowing classes to be deprecated. PR-URL: #7690 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <mic.besace@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2016-09-30T17:31:11Z

this is not landing cleanly on v4.x, would someone be willing to backport?

MylesBorins · 2016-09-30T21:38:32Z

this will need to be re-implemented without Reflect to work on v4.x

please feel free to submit a backport if you are able to

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label Jul 12, 2016

cjihrig reviewed Jul 13, 2016
View reviewed changes

targos reviewed Jul 13, 2016
View reviewed changes

cjihrig reviewed Jul 14, 2016
View reviewed changes

targos reviewed Jul 14, 2016
View reviewed changes

util: allow deprecate on classes

bec56bc

Classes objects cannot be called without new. util.deprecate executes 'fn.apply'. Therefore, classes cannot be deprecated with util.deprecate. This uses 'new.target' to detect (when process is defined) calls to constructor to allow deprecate on class.

jasnell reviewed Jul 29, 2016
View reviewed changes

cjihrig closed this Aug 4, 2016

cjihrig mentioned this pull request Aug 8, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

MylesBorins added the lts-watch-v4.x label Sep 30, 2016

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Sep 30, 2016

		const deprecated = util.deprecate(deprecatedClass, 'deprecatedClass is deprecated.');

		class subclass extends deprecated {


		const deprecated = util.deprecate(deprecatedClass, 'deprecatedClass is deprecated.');

		new deprecated();

util: allow deprecate on classes #7690

util: allow deprecate on classes #7690

Conversation

vdeturckheim commented Jul 12, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdeturckheim commented Jul 13, 2016

cjihrig commented Jul 13, 2016

Choose a reason for hiding this comment

vdeturckheim Jul 13, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fishrock123 commented Jul 13, 2016

Choose a reason for hiding this comment

cjihrig commented Jul 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjihrig commented Jul 14, 2016

targos commented Jul 14, 2016 • edited Loading

targos commented Jul 14, 2016 • edited Loading

vdeturckheim commented Jul 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Jul 14, 2016

vdeturckheim commented Jul 14, 2016 • edited Loading

targos commented Jul 14, 2016

vdeturckheim commented Jul 19, 2016

targos commented Jul 19, 2016

Choose a reason for hiding this comment

jasnell commented Jul 29, 2016

jasnell commented Aug 1, 2016

vdeturckheim commented Aug 2, 2016

addaleax commented Aug 2, 2016

cjihrig commented Aug 2, 2016

vdeturckheim commented Aug 2, 2016 • edited Loading

addaleax commented Aug 2, 2016

vdeturckheim commented Aug 2, 2016

cjihrig commented Aug 4, 2016

MylesBorins commented Sep 30, 2016

MylesBorins commented Sep 30, 2016

vdeturckheim commented Jul 12, 2016 •

edited

Loading

vdeturckheim Jul 13, 2016 •

edited

Loading

targos commented Jul 14, 2016 •

edited

Loading

targos commented Jul 14, 2016 •

edited

Loading

vdeturckheim commented Jul 14, 2016 •

edited

Loading

vdeturckheim commented Aug 2, 2016 •

edited

Loading