fix: Remove legacy spawnSync feature detection
#87
Conversation
This commit removes the checks used to detect and use `spawnSync` in Node <= 0.10. `spawnSync` is exposed since Node 0.11.
demurgos
force-pushed
the
legacy-spawn-sync-check
branch
from
6f3f4c1
to
f3fb337
Compare
| try { | ||
| spawnSyncBinding = process.binding('spawn_sync') | ||
| } catch (e) {} | ||
| const spawnSyncBinding = process.binding('spawn_sync') |
There was a problem hiding this comment.
I remember reading about possible deprecations but haven't heard more about it. Do you know how soon it may be deprecated? Node 13? Node 14?
Generally, I don't like that this library use Node's internals and monkey-patches them because it is fragile and has bad encapsulation. It is possible to patch the spawn function at a higher level: I would prefer to move into this direction. The internal patching could be kept for the moment, but ultimately I'd like to deprecate it. This progression would be similar to graceful-fs: it initially patched Node's internals but then moved to an "external wrapper" design.
There was a problem hiding this comment.
process.binding hit 'documentation only' deprecation in 10.9.0. Since 11.12.0 it is a 'pending deprecation', so node --pending-deprecation will show a warning. I don't have any information on when it will be escalated to full deprecation. I raised this module as a potential issue at nodejs/node#27344. I'm not entirely sure the 'external wrapper' way of graceful-fs will really work, the idea is to 'hijack' calls to spawn / spawnSync. Maybe the NODE_OPTIONS environment will work, I haven't had a chance to give that much thought yet.
https://nodejs.org/dist/latest-v11.x/docs/api/deprecations.html#DEP0111
There was a problem hiding this comment.
Oh, thank you very much for the info: I wasn't aware that it already started being deprecated.
An external wrapper would be a function that changes the options and then passes them to require("child_process").spawn, without ever changing the internals. But it only solves the issue at the top level. To wrap processes deep in the process tree (and hooking into code you don't control) I still patch the internals.
It's a tough problem.
There was a problem hiding this comment.
I opened an issue to discuss this deprecation: #89
This commit removes the checks to detect and use
spawnSyncin Node <= 0.10.spawnSyncis exposed since Node 0.11.