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

Jump to bottom

Fixing git style subcommands on Windows & allowing subcommands to come from dependency modules #604

Closed
wants to merge 17 commits into from
Closed

Fixing git style subcommands on Windows & allowing subcommands to come from dependency modules #604

Show file tree
Hide file tree
Changes from all 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…
Dec 15, 2016
dc70f71
adding find-module-bin dependency
Dec 15, 2016
3109d57
bumping find-module-bin
Dec 15, 2016
42a9d8e
fixing git subcommands on windows
Feb 7, 2017
fc6c364
bumping find-module-bin for es5 compat
Feb 7, 2017
7368f37
Merge branch 'master' into master
mateodelnorte Jul 11, 2017
3f5e2a9
using current convention for spawn
Jul 13, 2017
9a628f0
Merge branch 'master' into master
mateodelnorte Nov 21, 2017
ee4c7bf
Merge branch 'master' into master
mateodelnorte Dec 12, 2017
09ee7b2
updating with upstream changes, again
Apr 24, 2018
245bc4c
fixing lint errors. adding lint:fix target
Apr 24, 2018
7e22105
In Windows, "bat" scripts don't need interpreter
jsuhard Apr 30, 2018
ca9d8c6
Merge code for win32 and others platforms which is the same now
jsuhard Apr 30, 2018
3cb9f00
Style
jsuhard Apr 30, 2018
73a0e7f
Merge pull request #1 from jsuhard/master
mateodelnorte Apr 30, 2018
57b5e93
fixing tab->spaces to pass eslint
Apr 30, 2018
fc217f4
refactoring win32 platform test to variable for readability
May 1, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 11 additions & 12 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Copy link
Collaborator

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

Copy link
Author

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?

Copy link
Author

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.

Copy link
Author

Choose a reason for hiding this comment

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

@vanesyan

Copy link
Author

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.

Copy link

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?

var dirname = path.dirname;
var basename = path.basename;
var fs = require('fs');
Expand Down Expand Up @@ -541,29 +542,27 @@ Command.prototype.executeSubCommand = function(argv, args, unknown) {

// whether bin file is a js script with explicit `.js` extension
var isExplicitJS = false;
var isWindows = process.platform === 'win32';
if (exists(localBin + '.js')) {
bin = localBin + '.js';
isExplicitJS = true;
} else if (exists(localBin)) {
} else if (isWindows ? exists(localBin + '.cmd') : exists(localBin)) {
bin = localBin;
} else {
bin = findModuleBin(isWindows ? bin + '.cmd' : bin);
}

args = args.slice(1);

var proc;
if (process.platform !== 'win32') {
if (isExplicitJS) {
args.unshift(bin);
// add executable arguments to spawn
args = (process.execArgv || []).concat(args);
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', customFds: [0, 1, 2] });
}
proc = spawn(process.argv[0], args, { stdio: 'inherit', customFds: [0, 1, 2] });
} else {
args.unshift(bin);
proc = spawn(process.execPath, args, { stdio: 'inherit' });
proc = spawn(bin, args, { stdio: 'inherit', customFds: [0, 1, 2] });
}

var signals = ['SIGUSR1', 'SIGUSR2', 'SIGTERM', 'SIGINT', 'SIGHUP'];
Expand Down