Util(.is*) Deprecations #1301

Fishrock123 · 2015-03-31T15:33:33Z

Extension of #743 / #822 / #1295

Deprecates any util.is* function that are not used in core.

Also adds some cleanup for in-util deprecations.

WIP and pending much discussion I'm sure. Might as well just add other util deprecations here also.

cc @iojs/tc - @iojs/collaborators - @ceejbot - (everyone really..)

seishun · 2015-03-31T16:45:16Z

+1 If it can't be fixed, then kill it.

yosuke-furukawa · 2015-03-31T16:50:15Z

-1. that is going too far.
If util.is* functions have bugs, we should fix them. if the fix breaks compatibility, update major version.
We should not remove capabilities casually. If we deprecate functions, we should describe migration path clearly.

Fishrock123 · 2015-03-31T17:10:38Z

If util.is* functions have bugs, we should fix them. if the fix breaks compatibility, update major version.

Deprecating is surely less painful. Removal would be.. eventually.

Besides, most of these things aren't even used by core, and most of them are better done verbosely anyways.

bnoordhuis · 2015-03-31T17:12:16Z

lib/util.js

 function isPrimitive(arg) {
  return arg === null ||
         typeof arg !== 'object' && typeof arg !== 'function';
 }
 exports.isPrimitive = isPrimitive;

+// still used in buffer and child_process


Can be replaced in those files with Buffer.isBuffer().

Right, will do.

tellnes · 2015-03-31T17:23:39Z

+1

Maybe we should consider moving the rest of the functions to the new internal/ folder and deprecate those also?

evanlucas · 2015-03-31T17:26:27Z

I'm +1 on this. @isaacs core-util-is package is a suitable replacement in user-land IMO

ceejbot · 2015-03-31T17:30:57Z

+1 to this approach. If fixing is impossible, move users away from these functions gently.

thefourtheye · 2015-03-31T18:18:31Z

lib/util.js


+// note: the isRegExp function is still used here in util
 function isRegExp(re) {
  return re !== null && typeof re === 'object' &&
         objectToString(re) === '[object RegExp]';


Wouldn't the objectToString be sufficient here? Why do we need the other two checks?

rvagg · 2015-04-01T00:12:35Z

@Fishrock123 could you possibly find the TC meeting where util.is was discussed and reference it here for us perhaps? It may alleviate concerns by the -1ers here to see that the TC is fairly aligned on util.is*() generally being a mistake.

Fishrock123 · 2015-04-01T00:36:58Z

Only TC meeting reference I can find is here: TC 2015-02-18
(synopsis: punt, requires major for original fix)

Maybe you are referring to comments here? #822 (comment)

brendanashworth · 2015-04-01T01:02:35Z

lib/util.js

@@ -504,49 +505,60 @@ const isArray = exports.isArray = Array.isArray;
 function isBoolean(arg) {
  return typeof arg === 'boolean';
 }
-exports.isBoolean = isBoolean;
+exports.isBoolean = deprecate(isBoolean
+  , 'util.is* functions are deprecated, please use a user-land alternative.');


Would it be better for these to write it out for each, e.g., util.isBoolean is deprecated, please use a user-land alternative.?

Maybe, I just copy-pasta'd these.

Also though, it may be worth messaging that all of these have been deprecated? I'm not so sure.

I think its better to be explicit. If someone is using additional is*, it'll bubble up somewhere else.

yosuke-furukawa · 2015-04-01T01:28:11Z

I also think util.is* is not suitable in core. core library should be simple and modular.
BUT, I don't agree the idea that "if we cannot fix the functions, then deprecate them and delegate to others".

We should consider better migration path. For example, we maintain core-util-is libraries like readable-stream.

We also discuss officail userland module in here #1019
When the official userland module discussion is clear, we can deprecate util.is* functions and write more kind messages.

brendanashworth · 2015-04-08T07:27:57Z

I propose that this is voted on in the next TC meeting because:

The previous discussion punted discussion of an actual solution, it just expressed general "ew"
I think @yosuke-furukawa has a good point in establishing a migration path out; what do we do when we realize used parts in core just have to go?
Ever since Confusing documentation or bad implementation for util.isObject #743 this has been relatively controversial, though minor, and I think needs the TC to say "this is where we want to go with this"
No consensus here

silverwind · 2015-04-09T18:37:15Z

Noticed some tests also rely on the logging functions that are deprecated. Should probably run a find/replace on all js files.

Edit: Disregard me, these were already deprecated before.

feross · 2015-04-12T22:58:50Z

lib/util.js

@@ -573,7 +585,8 @@ exports.isPrimitive = isPrimitive;
 function isBuffer(arg) {
  return arg instanceof Buffer;
 }
-exports.isBuffer = isBuffer;
+exports.isBuffer = deprecate(isBuffer,
+    'util.isBuffer is deprecated, please use a user-land alternative.');


This should recommend using Buffer.isBuffer, not a user-land alternative.

Ooops. Honestly, I don't even know why we use Buffer.isBuffer either... it's just instannceof Buffer...

I'm not sure why Buffer.isBuffer was added initially. That'd be interesting to find out.

That said, Buffer.isBuffer has been incredibly useful to me in writing buffer for browserify. The Buffer constructor actually returns a Uint8Array for performance reasons, but with all the buffer methods attached as properties. Works great, except for one edge case – instanceof Buffer doesn't work anymore. So, I've been recommending that people use Buffer.isBuffer and that's worked well. :-)

Eventually, I can just return a proper Buffer that's a subclass of Uint8Array (arrays are finally subclassable!) and instanceof Buffer will start working.

cc @trevnorris?

I'm not sure why Buffer.isBuffer was added initially. That'd be interesting to find out.

Once upon a time there existed the SlowBuffer class. It didn't inherit from Buffer so instances were buffers but not instanceof Buffer.

SlowBuffer still exists but it's more or less obsolete. It inherits from Buffer now so instanceof checks work.

As to why it didn't before, I don't exactly remember but it was probably accidental. SlowBuffer was an implementation detail - it was the backing store for normal buffers - and not something users normally interacted with directly.

Thanks @bnoordhuis. That was before my time.

OK, should I use instanceof Buffer or better Buffer.isBuffer(...) instead of util.isBuffer in Node 0.12.7 and above?

You can also use Buffer.isBuffer which is not deprecated.
On Thu, Nov 12, 2015 at 3:55 AM hellboy81 notifications@github.com wrote:

In lib/util.js
#1301 (comment):

@@ -573,7 +585,8 @@ exports.isPrimitive = isPrimitive;
function isBuffer(arg) {
return arg instanceof Buffer;
}
-exports.isBuffer = isBuffer;
+exports.isBuffer = deprecate(isBuffer,

'util.isBuffer is deprecated, please use a user-land alternative.');

OK, should I use instanceof Buffer instead of util.isBuffer in Node 0.12.7
and above?

—
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/1301/files#r44648758.

And isBuffer is better than instanceof Buffer ?

They are exactly the same.
On Thu, Nov 12, 2015 at 4:38 AM hellboy81 notifications@github.com wrote:

In lib/util.js
#1301 (comment):

@@ -573,7 +585,8 @@ exports.isPrimitive = isPrimitive;
function isBuffer(arg) {
return arg instanceof Buffer;
}
-exports.isBuffer = isBuffer;
+exports.isBuffer = deprecate(isBuffer,

'util.isBuffer is deprecated, please use a user-land alternative.');

And isBuffer is better than instanceof Buffer ?

—
Reply to this email directly or view it on GitHub
https://github.com/nodejs/node/pull/1301/files#r44651882.

feross · 2015-04-12T23:01:56Z

Instead of suggesting a user-land alternative for some of these, can we just suggest the actual code required? It's more helpful than sending the user off to search npm. Like this:

util.isNull(arg) // "util.isNull is deprecated, please use `arg === null` instead"
util.isBoolean(arg) // "util.isBoolean is deprecated, please use `typeof arg === 'boolean'` instead"

Fishrock123 · 2015-04-12T23:27:59Z

Instead of suggesting a user-land alternative for some of these, can we just suggest the actual code required.

I agree.

jbergstroem · 2015-04-13T04:01:46Z

I like this too. Who wants to bake the patch? :)

yosuke-furukawa · 2015-04-14T08:52:10Z

Instead of suggesting a user-land alternative for some of these, can we just suggest the actual code required?

Yes, my concern is the deprecation message like use user-land module.
The deprecation message should have concrete migration path.

util.isNull(arg) // "util.isNull is deprecated, please use `arg === null` instead"
util.isBoolean(arg) // "util.isBoolean is deprecated, please use `typeof arg === 'boolean'` instead"

YES. This message is nice. Developers could understand the migration path.

evanlucas · 2016-02-01T21:58:58Z

They are already deprecated in the docs though https://nodejs.org/dist/latest-v5.x/docs/api/util.html#util_util_iserror_object

Fishrock123 · 2016-02-01T22:01:05Z

@evanlucas ... oh.

Then yes this should happen in v6 I think?

thefourtheye · 2016-02-01T23:39:45Z

@Fishrock123 yes. Yay :)

silverwind · 2016-02-15T10:28:49Z

Changing to major so it can land anytime now.

thefourtheye · 2016-02-15T10:33:02Z

please use a user-land alternative - ~~Should~~Do we really ~~have~~need that?

silverwind · 2016-02-15T10:40:46Z

please use a user-land alternative - Should we really have that?

Not a fan of that, too. The term user-land is probably not immediately clear, and if I wouldn't know better, I'd associate it with stuff running outside the kernel. I'd also be in favor of dropping please. While it's nice to be nice, I think we have more deprecation messages in place that aren't this nice.

I'd suggest util.isBoolean is deprecated, use typeof instead.

feross · 2016-02-15T22:17:48Z

@silverwind Agree. To improve further, what about suggesting the actual code:

util.isNull(arg) // "util.isNull is deprecated, use `arg === null` instead."
util.isBoolean(arg) // "util.isBoolean is deprecated, use `typeof arg === 'boolean'` instead"

seishun · 2016-02-15T22:26:11Z

@feross What about util.isObject? Should it reflect its actual behavior ("use arg && typeof arg == 'object'") or the behavior it should have had ("use arg && typeof arg == 'object' || typeof arg == 'function'")?

jasnell · 2016-04-04T22:08:56Z

@Fishrock123 ... is this still something that should land before v6 is cut?

mscdex · 2016-04-08T14:06:10Z

I'm a little confused about the deprecations, it seems like with some of the is* functions checking the type at the c++ layer, that those checks are "hard" to get right and hence why they are not simply checked with Object.prototype.toString.call(). That being the case, how would userland be able to definitively know the type of an object without writing their own binding that uses the v8 API?

cjihrig · 2016-04-08T14:26:36Z

I have to say, the checks that have been moved to C++ are likely helpful to userland. When we originally called to deprecate them, they were still done in JS. Perhaps we should reconsider at least those ones. I still don't see a need for something like isNull() though.

evanlucas · 2016-04-08T14:28:33Z

It already exists in userland though https://github.com/evanlucas/v8is

cjihrig · 2016-04-08T14:37:04Z

It already exists in core too. And people are already using it. Just my opinion though.

mscdex · 2016-04-08T14:41:51Z

@evanlucas IMHO any time you can avoid having to do compilation the better.

I would like to see the methods that can reliably check the type at the C++ layer get undeprecated, with a possible explanation in the docs as to why they exist and/or were undeprecated.

evanlucas · 2016-04-08T15:07:36Z

Yea, both of you make fair points. It would be nice to have those without a compile...

Fishrock123 · 2016-04-08T16:25:12Z

Right, so this existed at a time when we were not doing c++ checks...

I think we should keep at least the ones only able to be properly checked natively. I'm not sure about the others. Untagging from the milestone.

If we are going to keep these we should then un-deprecate them in the docs.

As a note, the combined isNullOrUndefined() and isPrimitive() checks stand out as different from the rest, and any others that aren't c++ reduce to simple typeof or === checks. (Except isObject(), I guess.)

thefourtheye · 2016-04-08T16:43:16Z

This util module is for the core, unless the core needs them let's remove them.

ChALkeR · 2016-04-13T21:05:03Z

Grep for util\.isRegExp: https://gist.github.com/ChALkeR/e9ef200e968c654c65bf9bd6020654bd

Note that this is partial, as it doesn't include require('util').isRegExp, and other names. This should cover the most part, though.

Fishrock123 · 2016-04-15T20:35:13Z

moving to #6235

util: cleanup in-util deprecation

281c44e

Fishrock123 added util Issues and PRs related to the built-in util module. discuss Issues opened for discussions and feedbacks. labels Mar 31, 2015

Fishrock123 modified the milestone: 2.0.0 Mar 31, 2015

vkurchatkin added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 31, 2015

bnoordhuis reviewed Mar 31, 2015
View reviewed changes

thefourtheye reviewed Mar 31, 2015
View reviewed changes

brendanashworth reviewed Apr 1, 2015
View reviewed changes

brendanashworth added the tc-agenda label Apr 8, 2015

rvagg mentioned this pull request Apr 8, 2015

io.js TC Meeting 2015-04-08 #1369

Closed

Fishrock123 added 2 commits April 8, 2015 10:34

util: deprecate most of utils.is*

d2fe24e

util: don't bother checking types before objectToString()

50ad0cc

Fishrock123 force-pushed the util-deprecations branch from 6b6ff46 to 50ad0cc Compare April 8, 2015 14:38

feross reviewed Apr 12, 2015
View reviewed changes

silverwind added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 15, 2016

thefourtheye mentioned this pull request Feb 24, 2016

doc: note util.isError() @@toStringTag limitations #5414

Merged

4 tasks

Fishrock123 removed this from the 6.0.0 milestone Apr 8, 2016

Fishrock123 added the discuss Issues opened for discussions and feedbacks. label Apr 8, 2016

Fishrock123 added the ctc-agenda label Apr 8, 2016

Fishrock123 removed ctc-agenda discuss Issues opened for discussions and feedbacks. labels Apr 15, 2016

Fishrock123 mentioned this pull request Apr 15, 2016

util: deprecate most of util.is*() #6235

Closed

4 tasks

Fishrock123 closed this Apr 15, 2016

Fishrock123 deleted the util-deprecations branch April 15, 2016 20:35

Util(.is*) Deprecations #1301

Util(.is*) Deprecations #1301

Conversation

Fishrock123 commented Mar 31, 2015

seishun commented Mar 31, 2015

yosuke-furukawa commented Mar 31, 2015

Fishrock123 commented Mar 31, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tellnes commented Mar 31, 2015

evanlucas commented Mar 31, 2015

ceejbot commented Mar 31, 2015

Choose a reason for hiding this comment

rvagg commented Apr 1, 2015

Fishrock123 commented Apr 1, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosuke-furukawa commented Apr 1, 2015

brendanashworth commented Apr 8, 2015

silverwind commented Apr 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feross commented Apr 12, 2015

Fishrock123 commented Apr 12, 2015

jbergstroem commented Apr 13, 2015

yosuke-furukawa commented Apr 14, 2015

evanlucas commented Feb 1, 2016

Fishrock123 commented Feb 1, 2016

thefourtheye commented Feb 1, 2016

silverwind commented Feb 15, 2016

thefourtheye commented Feb 15, 2016

silverwind commented Feb 15, 2016

feross commented Feb 15, 2016

seishun commented Feb 15, 2016

jasnell commented Apr 4, 2016

mscdex commented Apr 8, 2016

cjihrig commented Apr 8, 2016

evanlucas commented Apr 8, 2016

cjihrig commented Apr 8, 2016

mscdex commented Apr 8, 2016

evanlucas commented Apr 8, 2016

Fishrock123 commented Apr 8, 2016

thefourtheye commented Apr 8, 2016

ChALkeR commented Apr 13, 2016

Fishrock123 commented Apr 15, 2016