Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix node.cmd white space paths #51

Closed
wants to merge 4 commits into from
Closed

Fix node.cmd white space paths #51

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fe187da
Fix paths with blanks on windows (nyc)
equinoxel May 4, 2017
f67ae96
Bump version
equinoxel May 4, 2017
6edd714
Typo
equinoxel May 4, 2017
19ec23d
Bump version
equinoxel May 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,11 +150,15 @@ function mungeCmd (workingDir, options) {

var m = command.match(re)
var replace
var nodeCmd = workingDir + '/node.cmd'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be \\node.cmd?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily. afaik, JS on windows can parse "/" paths properly. One can use e.g. the "path" package:

var path = require("path");
var absolutePath = path.resolve("Relative file path");

if(workingDir.indexOf(' ') !== -1) {
nodeCmd = ' "' + nodeCmd + '"'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to not just always wrap the nodeCmd in quotes for Windows?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. I don't think there are side-effects.

}
if (m) {
options.originalNode = m[2]
replace = m[1] + workingDir + '/node.cmd' + m[3] + m[4]
options.args[cmdi + 1] = m[1] + m[2] + m[3] +
' "' + workingDir + '\\node"' + m[4]
nodeCmd + m[4]
} else {
// XXX probably not a good idea to rewrite to the first npm in the
// path if it's a full path to npm. And if it's not a full path to
Expand All @@ -165,7 +169,7 @@ function mungeCmd (workingDir, options) {

var npmPath = whichOrUndefined('npm') || 'npm'
npmPath = path_.dirname(npmPath) + '\\node_modules\\npm\\bin\\npm-cli.js'
replace = m[1] + workingDir + '/node.cmd' +
replace = m[1] + nodeCmd +
' "' + npmPath + '"' +
m[3] + m[4]
options.args[cmdi + 1] = command.replace(npmre, replace)
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "spawn-wrap",
"version": "1.3.4",
"version": "1.3.5-a",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to bump the version, we can do this when we release the update.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

"description": "Wrap all spawned Node.js child processes by adding environs and arguments ahead of the main JavaScript file argument.",
"main": "index.js",
"dependencies": {
Expand Down