Add mutable references for argv, execArgv and env without ST #26
Conversation
src/Node/Process.js
Outdated
| return function (dest) { | ||
| return function () { | ||
| dest.splice(0); | ||
| Array.prototype.push.apply(dest, source); |
There was a problem hiding this comment.
I think this is unnecessarily complex and problematic if the array is very large; I’d prefer to just do process.env = xs here instead.
There was a problem hiding this comment.
This would be for args though. Worried that if we're using node libs that has the line this.myArgs = process.args, won't it break if we don't do this?
There was a problem hiding this comment.
Oh right, I see. I mean, Array.prototype.push.apply is best avoided in general. If you really need to treat process.argv as a mutable array then I guess your previous formulation with STArray is preferable. But the more I think about it the more I reckon I don’t want to provide this API at all, especially since nobody has asked for it yet.
There was a problem hiding this comment.
Normally though, the mutation on argv is just by pushing and popping rather than replacing the entire thing. I can change the public api's to replace this. Then again, at that point, I'm thinking the ST stuff would be preferable.
There was a problem hiding this comment.
Yes I agree, I think if you’re treating it as a mutable reference I think the ST stuff is preferable. However, I don’t think it is appropriate for this library to take a stance on how argv should be treated (ie should it be treated as a mutable reference or should you overwrite the whole property), which is why I no longer want to provide any API for writing it here.
There was a problem hiding this comment.
Agreed. It did feel a bit off. Removing it then.
Note though that we're treating env differently and allowing setting / unsetting.
There was a problem hiding this comment.
Yes, that’s a good point. I think in the case of env it’s an easier call to make, since process.env isn’t quite a real object because it has its own special behaviour, like being able to set a value on process.env.FOO and then read it again as process.env.foo on Windows, and so it probably shouldn’t be overwritten. AFAICT process.argv is just a normal array though.
There was a problem hiding this comment.
I've removed the setters. Let me know if there are other things to change. :)
This reverts commit f0f96c7.
|
Thanks! |
Alternative to #25 without the ffi as suggested in #8 (comment).
Fixes #8