-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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.
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
Fixing git style subcommands on Windows & allowing subcommands to come from dependency modules #604
Changes from 6 commits
7be74e2
dc70f71
3109d57
42a9d8e
fc6c364
7368f37
3f5e2a9
9a628f0
ee4c7bf
09ee7b2
245bc4c
7e22105
ca9d8c6
3cb9f00
73a0e7f
57b5e93
fc217f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ | |
var EventEmitter = require('events').EventEmitter; | ||
var spawn = require('child_process').spawn; | ||
var path = require('path'); | ||
var findModuleBin = require('find-module-bin'); | ||
var dirname = path.dirname; | ||
var basename = path.basename; | ||
var fs = require('fs'); | ||
|
@@ -528,8 +529,10 @@ Command.prototype.executeSubCommand = function(argv, args, unknown) { | |
if (exists(localBin + '.js')) { | ||
bin = localBin + '.js'; | ||
isExplicitJS = true; | ||
} else if (exists(localBin)) { | ||
} else if (process.platform === 'win32' ? exists(localBin + '.cmd') : exists(localBin)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please put it to own variable, for readability purpose There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure thing. |
||
bin = localBin; | ||
} else { | ||
bin = findModuleBin(process.platform === 'win32' ? bin + '.cmd' : bin) | ||
} | ||
|
||
args = args.slice(1); | ||
|
@@ -546,8 +549,15 @@ Command.prototype.executeSubCommand = function(argv, args, unknown) { | |
proc = spawn(bin, args, { stdio: 'inherit', customFds: [0, 1, 2] }); | ||
} | ||
} else { | ||
args.unshift(bin); | ||
proc = spawn(process.execPath, args, { stdio: 'inherit'}); | ||
if (isExplicitJS) { | ||
args.unshift(bin); | ||
// add executable arguments to spawn | ||
args = (process.execArgv || []).concat(args); | ||
|
||
proc = spawn('node', args, { stdio: 'inherit'}); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As for 'node', can you do it like https: //github.com/tj/commander.js/blob/master/index.js#L544? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be good to go! Take a peek. |
||
} else { | ||
proc = spawn(bin, args, { stdio: 'inherit'}); | ||
} | ||
} | ||
|
||
var signals = ['SIGUSR1', 'SIGUSR2', 'SIGTERM', 'SIGINT', 'SIGHUP']; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,5 +25,7 @@ | |
"files": [ | ||
"index.js" | ||
], | ||
"dependencies": {} | ||
"dependencies": { | ||
"find-module-bin": "0.0.2" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please inline the module, we trying to keep commander.js dependencies-free
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a curiosity – What's the reason for wanting no dependencies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find-module-bin uses global-modules, to look up the appropriate os-dependent location of globally installed node modules. this won't be a simple pull-function-up refactor, as the os-specific logic is hidden in here.
perhaps we can merge this and set the refactoring as tech-debt?
personally, I think the reuse is better for the community. you don't want a massive explosion of dependencies, but having well managed dependencies can make your code more DRY and keeps the potential for the dependent packages to continue to evolve and solidify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vanesyan
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vanesyan I don't think it makes sense to inline all of this.
This PR has been ready for merge for about a year and six months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @vanesyan @abetomo
My team is also interested in this fix.
Is there any reason why this can not be included with the dependency?