Conversation
18e5cb5 to
7073a5c
Compare
|
@TimothyGu thanks for the command. Fixed it. If someone could please review this, that would be much appreciated. |
test/shjs.js
Outdated
| path.resolve(__dirname, '../bin/shjs') + | ||
| ' ' + | ||
| path.resolve(__dirname, 'resources', 'shjs', name), { silent: true }); | ||
| var cmd = (process.platform === 'win32' ? 'node ' : '') + path.resolve(__dirname, '../bin/shjs'); |
There was a problem hiding this comment.
I'm not sure if I have already mentioned this, but the best way to spawn another node process is through process.argv[0]. On both Unix and Windows, node refers to the executable in PATH, while the user might have executed the script with the absolute path to another node executable. process.argv[0] guarantees that the current used Node.js is always used.
There was a problem hiding this comment.
Yeah, that works. So does process.execPath I believe. I'll adjust.
|
Updated the tests to use |
|
It looks like the tests aren't passing, otherwise looks good. |
|
Yeah, I see the issue. I just need to wrap process.execPath with JSON.stringify(). Or you can take out the last commit and merge that (it's only the execPath stuff that broke tests). Away from my computer at the moment |
95fed2c to
6a94114
Compare
|
@ariporad I just updated this. This should fix the issue. |
6a94114 to
d4f4ba5
Compare
|
LGTM, will merge once tests pass. |
d4f4ba5 to
db20ace
Compare
|
Sorry, just found another issue. Now it should be resolved. |
|
Merging this, since it LGTM |
This is a redo of #258, since it looks like it may have been abandoned.