-
-
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
Closed
Closed
Changes from 7 commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
7be74e2
adding ability to specify git style submodules inherited from depende…
dc70f71
adding find-module-bin dependency
3109d57
bumping find-module-bin
42a9d8e
fixing git subcommands on windows
fc6c364
bumping find-module-bin for es5 compat
7368f37
Merge branch 'master' into master
mateodelnorte 3f5e2a9
using current convention for spawn
9a628f0
Merge branch 'master' into master
mateodelnorte ee4c7bf
Merge branch 'master' into master
mateodelnorte 09ee7b2
updating with upstream changes, again
245bc4c
fixing lint errors. adding lint:fix target
7e22105
In Windows, "bat" scripts don't need interpreter
jsuhard ca9d8c6
Merge code for win32 and others platforms which is the same now
jsuhard 3cb9f00
Style
jsuhard 73a0e7f
Merge pull request #1 from jsuhard/master
mateodelnorte 57b5e93
fixing tab->spaces to pass eslint
fc217f4
refactoring win32 platform test to variable for readability
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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(process.argv[0], args, { stdio: 'inherit', customFds: [0, 1, 2] }); | ||
} else { | ||
proc = spawn(bin, args, { stdio: 'inherit'}); | ||
} | ||
} | ||
|
||
var signals = ['SIGUSR1', 'SIGUSR2', 'SIGTERM', 'SIGINT', 'SIGHUP']; | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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" | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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?