lib: runtime deprecated access to process.binding('util')

[jasnell]

The various isType() functions exposed by process.binding('util')
are already available via util.types, and the rest really aren't
safe for external use. Deprecate access with the intent of removing
access in the near future.

/cc @ljharb since I know lodash still references this for Node.js version < 10 (for those users still on < 10.x this deprecation message will never print anyway).

and @addaleax whom I know has opinions on this stuff ;-)

Signed-off-by: James M Snell jasnell@gmail.com

[ljharb]

Thanks for the ping. Is this just when accessing util via process.binding?

[jasnell]

Yes, it's only triggered when accessing process.binding('util') directly

[nodejs-github-bot]

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

[addaleax]

Soooo... I don't want to reiterate the exact same conversation we had in #37485, but I would stick to my original suggestion, which would be to not runtime-deprecate it and instead make it so that process.binding('util') returns { ...require('util').types } (or the currently available subset, if we're picky and are okay with a few more lines in our code), and just leave it like that forever. Is there anything speaking against that? The code required to implement that should be fairly trivial.

[jasnell]

I guess the bit I'm struggling with is why we would do that. require('util/types') is available and is what everyone should be using, and it covers more use cases than process.binding('util'). The lodash case has been the most prominent use of these internal APIs and that has already been handled in newer versions -- folks that are using old Node.js versions won't be impacted and folks using the newer versions are already using util/types. In other words, I'm not sure there's any actual benefit to modifying this.

Even with that said, a runtime deprecation of the current exports is still appropriate given that there are other methods and constants exposed here that do not make sense to expose in any scenario. The step after runtime deprecation does not have to be removal -- the step after deprecation could be easily be replacing the current process.binding('util') with an alias for util/types as suggested, in which case the status would change to "Legacy" and should remain undocumented.

tl;dr ... we can do both. runtime deprecate the current and replace it after.

[nodejs-github-bot]

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

[addaleax]

I guess the bit I'm struggling with is why we would do that. ``

Again – not trying to rehash the entire conversation here, but the point is that there’s still a decent amount of code that does use process.binding('util') for type checking, and whether that’s because people don’t upgrade deep dependencies is not actually relevant here. (Just like there is a decent amount of code that still uses atob and btoa, by the way… and we have just introduced those because of that!)

So: The reason why we do that is that we care about our users, which involves caring about backwards compatibility and making transitions between Node.js versions as smoothly as possible when it comes at trivial cost to us.

I do realize that it’s frustrating when somebody is objecting without putting in the work to provide an alternative, so: #37819

[ljharb]

Could we perhaps do both? it'd be two semver-majors, ofc, but at least then we can get there.

[jasnell]

I'm not going to worry about this. @addaleax's alternative PR is fine.