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

Request to undeprecate util.is* functions #43715

Closed
WillAvudim opened this issue Jul 7, 2022 · 24 comments
Closed

Request to undeprecate util.is* functions #43715

WillAvudim opened this issue Jul 7, 2022 · 24 comments
Labels
feature request Issues that request new features to be added to Node.js. util Issues and PRs related to the built-in util module.

Comments

@WillAvudim
Copy link

WillAvudim commented Jul 7, 2022

What is the problem this feature will solve?

I'd like to request to undeprecate all util.is* functions currently marked for deprecation, and keep them intact in all future versions of Node.js.

What is the feature you are proposing to solve the problem?

Reasons:

  1. util.is* functions (util.isObject, util.isNumber, util.isRegExp) are immensely useful and used throughout the codebase of pretty much every Node.js app.

  2. They are succinct and help to avoid unnecessary cognitive load, while the suggested alternatives are verbose and difficult to remember, and write correctly every single time.

Take for example util.isObject and compare the function isObject:

if (isObject(functionThatReturnsObject(param1, param2)) { ... }

VS what its documentation suggests I'd have to write instead:

if (functionThatReturnsObject(param1, param2) !== null && typeof functionThatReturnsObject(param1, param2) === 'object') { ... }

Do you really think that the second fragment is more obvious to any software engineer out there?

  1. The alternative implementation, e.g. lodash, is less efficient since it has to handle many corner cases not applicable to Node.js. And lodash detects Node.js and uses util.is* functions underneath for efficiency, e.g. https://github.com/lodash/lodash/blob/ddfd9b11a0126db2302cb70ec9973b66baec0975/lodash.js#L452

  2. They have great typescript annotations already written and maintained.

What alternatives have you considered?

No response

@WillAvudim WillAvudim added the feature request Issues that request new features to be added to Node.js. label Jul 7, 2022
@VoltrexKeyva VoltrexKeyva added the util Issues and PRs related to the built-in util module. label Jul 7, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jul 7, 2022

Context for deprecation: #2447

@WillAvudim
Copy link
Author

Oh, I'm not the only one! There are people saying pretty much the same on the thread, even more eloquently: #2447 (comment)

Basically, our message is:

  1. This functionality is super useful and wildly used.
  2. We love it, we rely on it in our every day coding, and we consider it to be an integral part of Node.js (this specific javascript execution environment).
  3. There are indeed alternatives out there that are simply neither as good nor efficient nor they are so easy-to-read and understand.

@benjamingr
Copy link
Member

I'm not sure why though since there are native ways to check for all of these?

The problem is that something like typeof foo === 'number' clearly checks if something is of a number type for example but when seeing util.isNumber (which literally just checks typeof === 'number' internally) it is unclear if further validations are performed, what the behavior of NaN is, what happens for BigInts etc.

Having our own similar but with possibly slightly different semantics methods for these things isn't really a positive in a time where we want to move to more standard JavaScript.

@WillAvudim
Copy link
Author

WillAvudim commented Jul 8, 2022

When one sees util.isNumber, one can rest assured that any number will be correctly detected on Node.js.

When one sees plain typeof value == 'number', then the question arises whether you also need (isObjectLike(value) && baseGetTag(value) == numberTag) to handle it correctly on Node.js or not (https://github.com/lodash/lodash/blob/ddfd9b11a0126db2302cb70ec9973b66baec0975/lodash.js#L12005).

In other words, typeof value == ' number' is tricky because it might not behave as you expect in all javascript environments. util.isNumber encapsulates that knowledge and implements the minimally sufficient check to detect the number for Node.js specifically (granted that we only use V8).

You can argue that one should remember what checks work for Node.js vs what checks are necessary for all other environments out there, but that doesn't scale, it would incur too much extra cognitive load.

@BridgeAR
Copy link
Member

BridgeAR commented Jul 8, 2022

I would argue that Number(5) is not a number but an object. As such, I would expect a isNumber() function to return false. As you can see, there are different opinions about what is considered right or wrong and being explicit with typeof value === 'number' is good for these circumstances.

@BridgeAR
Copy link
Member

BridgeAR commented Jul 8, 2022

The util.isNumber() function does in fact only check typeof and not for wrapped primitives.

@WillAvudim
Copy link
Author

Still, isNumber(value) is easier to type, read, and remember than typeof value === "number", and it encapsulates the knowledge of what would be the correct check for Node.js.

Imagine that you actually have to remember and type out the entire value !== null && typeof value === "object" every time you have to check for an object. Surely, isObject(value) beats it hands down, doesn't it?

@benjamingr
Copy link
Member

@WillAvudim I wrote the last comment after I checked the code and util.isNumber is literally typeof arg === 'number' in the code (as Ruben also pointed out).

The thing is -it isn't entirely clear what the semantics are for these utilities.

Let's take isObject for example:

  • Is a function an object? According to JavaScript it is, according to isObject it's not. You can argue that it's type is not an object but typeof is not the same as isX and (function() {}) instanceof Object returns true. With typeof the result is obvious.
  • What is something is an object from another realm (e.g. from a vm.runInNewContext)? What should isObject return?
  • Anything special for proxies?
  • Is null really conceptually an object since it represents the absence thereof and its typeof is Object?

I can come up with 10 more such questions for each helper basically :D

@WillAvudim
Copy link
Author

You just listed even more reasons to encapsulate all these decisions in a single standard util.isObject function. Think of it as a form of general consensus.

If you deprecate it, all of us would have to answer all these questions anew, and any npm install will be pulling not just 10,000 packages, but 10,000 packages with 10,000 (or even more) different implementations of isObject, many of them will be bugged, some are subtly so.

Let's make everyone type out value !== null && typeof value === "object" all the time, as an exercise. How many people will type it as value != null && typeof value == "object"? Is the latter form even correct? How many seconds it took you to arrive at a conclusion? What if they mistype it as value != undefined && typeof value == "object", is that still correct?

@BridgeAR
Copy link
Member

BridgeAR commented Jul 8, 2022

The deprecation of this is around for more than 10 major versions and I could not find a precedent where people were unhappy about it before in the last six years. As such, it is extremly unlikely to ever be changed again.
If you feel strongly about this, I suggest to actually add something like this to JavaScript as a language. Node.js itself is just one runtime and all other runtimes would either have no or an independent implementation. So please consider making a suggestion to TC39.

@WillAvudim
Copy link
Author

I definitely see a precedent in the change thread, and I believe, the majority of Node.js users would fully support this undeprecation (#2447 (comment)).

Do you have some sort of a voting mechanism to expose this decision publicly?

I disagree that it should be a javascript feature, runtime is the right place for it since we know that we only use V8 and can leverage this knowledge to make some checks more efficient.

@benjamingr
Copy link
Member

Do you have some sort of a voting mechanism to expose this decision publicly?

In a gist: Generally every collaborator can block or land code changes, if the change is objected to discussion is had until consensus is reached, if consensus cannot be reached the technical steering committee gets involved and if they can't agree there is a majority vote.

Community feedback is gathered through discussions, surveys etc. You can read more about the process in the repo docs.


As for the technical bits:

You just listed even more reasons to encapsulate all these decisions in a single standard util.isObject function. Think of it as a form of general consensus.

I listed reasons why we cannot reasonably encapsulate these decisions in a single standard util.isObject function that has unambiguous semantics.

I disagree that it should be a javascript feature, runtime is the right place for it since we know that we only use V8 and can leverage this knowledge to make some checks more efficient.

i'm... not sure what you mean. TC39 specifies ECMAScript which defines semantics for how code should behave not how that behavior should be implemented. The fact we use V8 is irrelevant to that.

@WillAvudim
Copy link
Author

The check value !== null && typeof value === "object" has no ambiguity. Everything that passes the check is considered to be an object as far as util.isObject() is concerned. The check is documented as part of util.isObject() documentation, there is no any semantic ambiguity or possibility for misbehavior. And it's a good thing that it doesn't consider functions to be objects, that's exactly the behavior we have all come to expect.

All these util functions have been around for long, they are widely used, well understood, super useful, let's consider it all settled at this point. Semantic ambiguity is resolved by documenting how exactly the check is performed. Why do you want to delete them?

Thank you for your answers! I'll look into those documents.

@aduh95
Copy link
Contributor

aduh95 commented Jul 8, 2022

they are widely used

Do we have data to back this up?

I disagree that it should be a javascript feature, runtime is the right place for it since we know that we only use V8 and can leverage this knowledge to make some checks more efficient.

Almost certainly V8 devs would do a better job at optimizing such an util function at the engine level than we would in Node.js at the JavaScript level. And same would apply for other JS engines.

@WillAvudim
Copy link
Author

241,660 references on github alone - https://github.com/search?p=99&q=util.isObject&type=Code
As an anecdotal evidence, every single project I've touched so far uses some of these functions.

I hope V8's TurboFan inlines it, so it just becomes a built-in check. May be they already do have some optimizations for typeof <> === "object", though that's beyond my expertise.

@Trott
Copy link
Member

Trott commented Jul 10, 2022

241,660 references on github alone - https://github.com/search?p=99&q=util.isObject&type=Code

There are a lot of false positives.

That search matches anything that uses the string "util" and the string "isObject" but it need not be "util.isObject".

A better (but still not correct) search is https://github.com/search?q=%22util.isObject%22&type=Code but that will match "util/isObject" etc. The GitHub search does not match punctuation. So, OK, that still matches 221,279 results.

But that includes markdown and HTML and anything.

And that also includes a lot of very old code. The search for "new Buffer()" has even more JavaScript results than "util.isObject" so I'm not sure that these GitHub search results are going to win the day here.

@devsnek
Copy link
Member

devsnek commented Jul 10, 2022

@Trott
Copy link
Member

Trott commented Jul 10, 2022

it is extremly unlikely to ever be changed again.

I agree with this assessment from @BridgeAR. I would be strongly opposed to changing this.

Use of these functions should be discouraged. There are idiomatic JavaScript ways to do these checks that allow you to make it clear what you're doing (e.g., are you excluding null from your object check or not?).

@WillAvudim
Copy link
Author

I agree with this assessment from @BridgeAR.

@BridgeAR assessment was based on the assumption that e.g. util.isObject identifies Javascript objects, which is indeed an ill-defined concept. However, that assumption is faulty. The only thing that util.isObject aims to establish is that typeof value returns the string "object", which lacks any sort of ambiguity and is strictly defined. Same is applicable to util.isNumber, etc.

it is extremly unlikely to ever be changed again.

I would be strongly opposed to changing this.

Let's judge the change by the outcome it would produce, not by dogmatic beliefs of the past. Sure, mistakes have been made, the functions have been marked as deprecated, dissent ignored. That doesn't mean that we want to make everyone write their own version of said functions, or endlessly repeat the same recondite formulas like value !== null && typeof value === "object" throughout the codebase.

Use of these functions should be discouraged. There are idiomatic JavaScript ways to do these checks that allow you to make it clear what you're doing (e.g., are you excluding null from your object check or not?).

Judging by the existing codebases, people tend to use util.is* rather than repeat these checks from scratch all over again. Codesearches above, while limited and not catching all their applications, still vouch for undeniable usefulness.

Sure, you can delete these functions, break lots of code in the wild, force everyone incorporate a clone of these functions into their codebases, and all for what? What are the benefits you are after? In what real-world scenario would you not check for null if you're expecting an object?

@Trott
Copy link
Member

Trott commented Jul 10, 2022

Sure, you can delete these functions, break lots of code in the wild, force everyone incorporate a clone of these functions into their codebases, and all for what?

No one is suggesting that is ever going to happen. Deprecation means discouraged, not slated for removal.

@WillAvudim
Copy link
Author

Oh, they shall never be deleted? I see. I'm sorry, I acted on the assumption that deprecation indicates imminent removal. This reminds me Google's modus operandi: Things are either deprecated or not yet implemented.

Closing this bug then. Thank you for all your responses and have a wonderful day!

@Trott
Copy link
Member

Trott commented Jul 10, 2022

Oh, they shall never be deleted? I see. I'm sorry, I acted on the assumption that deprecation indicates imminent removal.

To be fair, that's a common source of confusion and we (Node.js) could do more to clarify.

@aduh95
Copy link
Contributor

aduh95 commented Jul 10, 2022

We could use the Legacy status for those functions, that seems to be more suited than deprecation at this point.

@Trott
Copy link
Member

Trott commented Jul 10, 2022

We could use the Legacy status for those functions, that seems to be more suited than deprecation at this point.

I thought that the Legacy status specifically indicates that Node.js provides a new API, which isn't the case in this situation, but now that I look at the text, it just says "other APIs are available" which I guess doesn't mean necessarily APIs that Node.js provides. And even if it does, we can change the text.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests

7 participants