[Converge] child_process argument type checking
Closed this issue · 6 comments
These commits add argument type checking to methods within child_process. This needs to be reconciled with the current io.js behavior. The change introduces a new throw so there's a potential API compatibility issue.
/cc @trevnorris
a1b2875afd9affd3e0147a10f04a2daf8c598761 lint: fix lint issues- nodejs/node-v0.x-archive@a1b28758032a210250c86877898331ac4474f39b3a573c6 test: test all spawn parameter positions- nodejs/node-v0.x-archive@8032a2170dafa7b624abd43432e03304d65cc527fbecc11 child_process: check fork args is an array- nodejs/node-v0.x-archive@70dafa7e17c5a72b23f920f291d61f2780068c18768cb92 child_process: check execFile args is an array- nodejs/node-v0.x-archive@e17c5a7
@piscisaureus I think these are the commits you said you didn't bring over. My primary involvement in this was to make the code actually match the docs in terms of what args were or were not optional. But that fell into a can of worms because some vocal users wanted very specific type checking on the args. Which isn't a bad thing, but with four args, most optional, it gets a bit hairy. I leave it to you all to decide what to do about this.
And note that the originally claimed "API incompatiblity" was a LACK of a throw...
I think at least the tests should be ported over, since they demonstrate the very wide variety of calling conventions that are possible... what the correct behaviour should be for each one can then be discussed more readily.
@nodejs/tsc ... this may be one we need broader review on.
The changes look reasonable to me. If one of you turns this into a PR and cc's me, I'll sign off on it.
I'll be working on doing so
now active @ nodejs/node#2515