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

Open
wants to merge 17 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mateodelnorte

mateodelnorte commented Jan 16, 2017

This PR fixes git style subcommands to work correctly on Windows, and adds logic such that git style subcommands can be inherited from child dependencies, allowing you to break large sets of commands into a multi-repo project. (It should also allow for dynamically varying available subcommands according to what is currently installed under ./node_modules)

I built a command for managing multi-repo projects, which dogfoods this patch to manage its own repos as well: https://github.com/mateodelnorte/meta/blob/master/package.json#L38-L39. meta, itself, has only one command – meta, which loads all child commands by searching through it's node_modules for matching meta-* modules and registering them via their index.js.

This all works fine with the current commander.js, but when attempting to call them commander can't locate them. This patch ensures that commander can find the git subcommands and execute them.

meta --help

  Usage: meta [options] [command]


  Commands:

    git         manage your meta repo and child git repositories 
    npm         run npm commands against your meta and child repositories 
    help [cmd]  display help for [cmd]

  Options:

    -h, --help     output usage information
    -V, --version  output the version number

The git subcommand, above, is provided by https://github.com/mateodelnorte/meta-git.
The npm subcommand, above, is provided by https://github.com/mateodelnorte/meta-npm

mateodelnorte added some commits Dec 15, 2016

adding ability to specify git style submodules inherited from depende…
…ncies. enables plugin infrastructure where subcommands can be separated into different modules and repos
@mateodelnorte

This comment has been minimized.

mateodelnorte commented Jan 25, 2017

Do we actually care that this breaks old (v<=0.0.10) versions of node?

@mateodelnorte mateodelnorte changed the title from Allow git style subcommands to come from dependencies to Fixing git style subcommands on Windows & allowing subcommands to come from dependency modules Feb 7, 2017

@mateodelnorte

This comment has been minimized.

mateodelnorte commented Feb 15, 2017

PING PING

Are Node 0.8 and 0.6 support really required in commander.js? The only thing keeping this PR from passing all tests are engine restrictions in some dependencies.

Thanks.

@mateodelnorte

This comment has been minimized.

mateodelnorte commented May 12, 2017

Hi. Howdy. Hello there. This is a pretty popular package. Lots of PR's. Some are months old. Do people respond to them?

@abetomo

This comment has been minimized.

Collaborator

abetomo commented Jul 11, 2017

@mateodelnorte Would you please resolve the conflict?

@abetomo abetomo requested review from abetomo and vanesyan Jul 11, 2017

@mateodelnorte

This comment has been minimized.

mateodelnorte commented Jul 11, 2017

Conflicts resolved.

index.js Outdated
// add executable arguments to spawn
args = (process.execArgv || []).concat(args);

proc = spawn('node', args, { stdio: 'inherit'});

This comment has been minimized.

@abetomo

abetomo Jul 12, 2017

Collaborator

As for 'node', can you do it like https: //github.com/tj/commander.js/blob/master/index.js#L544?

This comment has been minimized.

@mateodelnorte

mateodelnorte Jul 13, 2017

Should be good to go! Take a peek.

@abetomo

This comment has been minimized.

Collaborator

abetomo commented Jul 12, 2017

Thank you!

@mateodelnorte

This comment has been minimized.

mateodelnorte commented Jul 14, 2017

Glad to help!

*not sure what's up with npm 5 not installing the single find-module-bin dependency. yarn installs it just fine.

@mateodelnorte

This comment has been minimized.

mateodelnorte commented Jul 24, 2017

Any timeline on when this will be published to npm? I think a simple rebuild will fix the build errors on node 8. Seemed to be a temporary problem with the registry. The dependency version def exists:

{ name: 'find-module-bin',
  description: 'scan known node_modules directories to find installation directory of a particular bin file',
  'dist-tags': { latest: '0.0.2' },
  versions: [ '0.0.0', '0.0.1', '0.0.2' ],
@mateodelnorte

This comment has been minimized.

mateodelnorte commented Nov 21, 2017

When is this going to be merged? The error happening in this build was a bug in npm5.

@mateodelnorte

This comment has been minimized.

mateodelnorte commented Nov 21, 2017

I've fixed the merge issue that happened in package.json as a result of this not being merged since January. Looks like a re-run of the builds works completely (because it was a bug in npm5).

Can we get this merged?

Thx

@mateodelnorte

This comment has been minimized.

mateodelnorte commented Mar 28, 2018

@abetomo Can we please slate this for merge? It's been well over a year.

@abetomo

This comment has been minimized.

Collaborator

abetomo commented Mar 29, 2018

@mateodelnorte
It was awaiting @vanesyan's approval.

@vanesyan
Could you please review it?

@mateodelnorte

This comment has been minimized.

mateodelnorte commented Apr 24, 2018

So... I updated this from upstream again. @abetomo @vanesyan

Can we get this merged?

jsuhard and others added some commits Apr 30, 2018

Merge pull request #1 from jsuhard/master
Fix issues #37 - Running any subcommand throws SyntaxError on Windows
@mateodelnorte

This comment has been minimized.

mateodelnorte commented Apr 30, 2018

ping ping

index.js Outdated
@@ -544,26 +545,23 @@ 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)) {

This comment has been minimized.

@vanesyan

vanesyan May 1, 2018

Collaborator

Can you please put it to own variable, for readability purpose

This comment has been minimized.

@mateodelnorte
@@ -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');

This comment has been minimized.

@vanesyan

vanesyan May 1, 2018

Collaborator

Can you please inline the module, we trying to keep commander.js dependencies-free

This comment has been minimized.

@mateodelnorte

mateodelnorte May 1, 2018

As a curiosity – What's the reason for wanting no dependencies?

This comment has been minimized.

@mateodelnorte

mateodelnorte May 1, 2018

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.

This comment has been minimized.

@mateodelnorte

This comment has been minimized.

@mateodelnorte

mateodelnorte Oct 2, 2018

@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.

@vanesyan

This comment has been minimized.

Collaborator

vanesyan commented May 1, 2018

Hello there! Sorry for make you waiting. Can you, please, resolve the comments in the review, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment