lib: runtime deprecate process.binding()

[RafaelGSS]

Following up @jasnell work on runtime deprecation of some modules. This pr deprecates the process.binding entirely, targetting Node.js 21 (non-LTS version).

I'm opening this PR just to see how feasible/impactful is this approach. I wonder if #37485 (review) is still an issue.

cc: @nodejs/tsc

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/modules
• @nodejs/startup

[bnoordhuis]

Doesn't this do the opposite of runtime deprecating?

[RafaelGSS]

This was emitting a warning whenever a runtime deprecated module is called with process.binding. Example, process.binding('async_wrap'). Now, all the modules are deprecated, so I removed them in favor of:

process.binding = deprecate(process.binding,
                              'process.binding() is deprecated. ' +
    'Please use public APIs instead.', 'DEP0111');

Otherwise, users were going to see two warning deprecations for the same operation.

[joyeecheung]

I think a less disruptive approach to deprecate process.binding() could be:

1. Use the legacyWrapperList mechanism to list the current process.binding() surface explicitly (in particular, stop adding more stuff to the surface, because we don't always pay attention to the additions to these bindings)
2. Deprecate and remove the problematic properties from the surface, providing alternatives if possible.
3. Keep surface as harmless as possible, and take things away when we find them problematic

Simply deprecating process.binding() entirely could be quite disruptive - for example, if a dependency of some kind of tool that spins off child processes frequently (e.g. for running tests) uses this API, which is not that rare, a runtime deprecation could result in a lot of noise for the user and forces them to set --no-deprecation to work around it, which in the end could cause more problems. It's also not entirely necessary because even though there are many problematic APIs in process.binding(), there are also a lot of ones that aren't very problematic (read-only or side-effect-free with non-sensitive data). We also haven't really looked into the surface and prepared enough alternatives for them, the "Please use public APIs instead" suggestion isn't really quite useful compared to the per-API suggestions that we theoretically should be able to make.

[mcollina]

lgtm

[targos]

To assess ecosystem impact, we could change it from warning to throwing an error and run CITGM.

[RafaelGSS]

I think a less disruptive approach to deprecate process.binding() could be:

1. Use the legacyWrapperList mechanism to list the current process.binding() surface explicitly (in particular, stop adding more stuff to the surface, because we don't always pay attention to the additions to these bindings)
2. Deprecate and remove the problematic properties from the surface, providing alternatives if possible.
3. Keep surface as harmless as possible, and take things away when we find them problematic

Simply deprecating process.binding() entirely could be quite disruptive - for example, if a dependency of some kind of tool that spins off child processes frequently (e.g. for running tests) uses this API, which is not that rare, a runtime deprecation could result in a lot of noise for the user and forces them to set --no-deprecation to work around it, which in the end could cause more problems. It's also not entirely necessary because even though there are many problematic APIs in process.binding(), there are also a lot of ones that aren't very problematic (read-only or side-effect-free with non-sensitive data). We also haven't really looked into the surface and prepared enough alternatives for them, the "Please use public APIs instead" suggestion isn't really quite useful compared to the per-API suggestions that we theoretically should be able to make.

So practically speaking, your suggestion is to keep doing what @jasnell was doing on #37576 and subsequent PRs?

My initial thought was to leverage that the next major release is a non-lts version and it's likely the best way to assess the real impact of that feature. If that proves not worth it, we can revert to Node.js 22 -- I know that that's far from ideal and can lead us to some non-great feedback from the community, but I feel that's the only way to really see the impact and plan what to do next.

To assess ecosystem impact, we could change it from warning to throwing an error and run CITGM.

Agree. Regardless if we're going to pursue this work or not, it's good to have a big picture. I'll run it right after fixing the test issues.

[RafaelGSS]

CITGM: https://ci.nodejs.org/job/citgm-smoker/3179/ (rebased on main)

[jasnell]

FWIW, I'm fine with the more disruptive approach on this. It's been long enough.

[mcollina]

lgtm

[aduh95]

@RafaelGSS can you please rebase?

[RafaelGSS]

Closing in favor of #50687