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

Util(.is*) Deprecations #1301

Closed
wants to merge 3 commits into from
Closed

Conversation

Fishrock123
Copy link
Contributor

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..)

@Fishrock123 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 Fishrock123 modified the milestone: 2.0.0 Mar 31, 2015
@vkurchatkin vkurchatkin added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 31, 2015
@seishun
Copy link
Contributor

seishun commented Mar 31, 2015

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

@yosuke-furukawa
Copy link
Member

-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
Copy link
Contributor Author

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.

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

// still used in buffer and child_process
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, will do.

@tellnes
Copy link
Contributor

tellnes commented Mar 31, 2015

+1

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

@evanlucas
Copy link
Contributor

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

@ceejbot
Copy link

ceejbot commented Mar 31, 2015

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


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

Choose a reason for hiding this comment

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

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

@rvagg
Copy link
Member

rvagg commented Apr 1, 2015

@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
Copy link
Contributor Author

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)

@@ -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.');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

@yosuke-furukawa
Copy link
Member

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
Copy link
Contributor

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

@silverwind
Copy link
Contributor

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.

@@ -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.');
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 caseinstanceof 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @bnoordhuis. That was before my time.

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

And isBuffer is better than instanceof Buffer ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Contributor

feross commented Apr 12, 2015

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
Copy link
Contributor Author

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

I agree.

@jbergstroem
Copy link
Member

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

@yosuke-furukawa
Copy link
Member

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
Copy link
Contributor

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

@Fishrock123
Copy link
Contributor Author

@evanlucas ... oh.

Then yes this should happen in v6 I think?

@thefourtheye
Copy link
Contributor

@Fishrock123 yes. Yay :)

@silverwind 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
@silverwind
Copy link
Contributor

Changing to major so it can land anytime now.

@thefourtheye
Copy link
Contributor

please use a user-land alternative - ShouldDo we really haveneed that?

@silverwind
Copy link
Contributor

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
Copy link
Contributor

feross commented Feb 15, 2016

@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
Copy link
Contributor

seishun commented Feb 15, 2016

@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
Copy link
Member

jasnell commented Apr 4, 2016

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

@mscdex
Copy link
Contributor

mscdex commented Apr 8, 2016

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
Copy link
Contributor

cjihrig commented Apr 8, 2016

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
Copy link
Contributor

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

@cjihrig
Copy link
Contributor

cjihrig commented Apr 8, 2016

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

@mscdex
Copy link
Contributor

mscdex commented Apr 8, 2016

@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
Copy link
Contributor

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

@Fishrock123 Fishrock123 removed this from the 6.0.0 milestone Apr 8, 2016
@Fishrock123 Fishrock123 added the discuss Issues opened for discussions and feedbacks. label Apr 8, 2016
@Fishrock123
Copy link
Contributor Author

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
Copy link
Contributor

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

@ChALkeR
Copy link
Member

ChALkeR commented Apr 13, 2016

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 Fishrock123 removed ctc-agenda discuss Issues opened for discussions and feedbacks. labels Apr 15, 2016
@Fishrock123
Copy link
Contributor Author

moving to #6235

@Fishrock123 Fishrock123 deleted the util-deprecations branch April 15, 2016 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

Successfully merging this pull request may close these issues.