lib: runtime deprecated access to process.binding('util') - #37787
Conversation
Sorry, something went wrong.
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.
Signed-off-by: James M Snell <jasnell@gmail.com>
|
Thanks for the ping. Is this just when accessing util via process.binding? |
Sorry, something went wrong.
|
Yes, it's only triggered when accessing process.binding('util') directly |
Sorry, something went wrong.
Sorry, something went wrong.
addaleax
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Sorry, something went wrong.
addaleax
left a comment
There was a problem hiding this comment.
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
Sorry, something went wrong.
|
Could we perhaps do both? it'd be two semver-majors, ofc, but at least then we can get there. |
Sorry, something went wrong.
|
I'm not going to worry about this. @addaleax's alternative PR is fine. |
Sorry, something went wrong.
Ref: #37485 (review) Ref: #37787 PR-URL: #37819 Refs: #37787 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
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