recursive-install icon indicating copy to clipboard operation
recursive-install copied to clipboard

Add `npm-recursive-test` command

Open dchambers opened this issue 7 years ago • 8 comments

This command complements the already existing npm-recursive-install command. The implementation could be made a lot more DRY, but this is enough to test whether it might be suitable for inclusion upstream.

dchambers avatar Oct 27 '16 14:10 dchambers

This looks like a pretty reasonable change to me. What's the purpose of the recursive-test.js file?

emgeee avatar Nov 08 '16 17:11 emgeee

Hi @emgeee, apologies for the slow response. The recursive-test.js file is only really there because I was trying to keep things as light-touch as possible so that I could present a more minimal diff. I'm happy to create a DRYer commit if you are happy in principle with the idea, or could potentially create a commit that allows any command to be recursively run, with care taken to make sure it's still easy for developers to use.

What are your thoughts?

dchambers avatar Nov 11 '16 09:11 dchambers

Ya know what, I just realized you snuck in a new feature in the form of being able to run npm test recursively as well! (that's why I was a little confused)

Given that, I definitely think things can be DRY'er, specifically recursive-test.js and recursive-install.js look to be pretty much exactly the same, except for which functions they call when entering each directory. I'd like to see the shared logic from these two files abstracted away into another file called something like executeCommandRecursively.js (or something like it) so that the contents of recursive-test.js and recursive-install.js could be replaced with something simple like

executeCommandRecursively((directory) => exec('npm install'))

This would make this script a lot more extensible and allow for someone to easily extent the module to, for example, recursively run yarn install

emgeee avatar Nov 11 '16 17:11 emgeee

Nice, I like the sound of all of that, and I was already wondering about creating a Yarn version, so it all fits together nicely.

I wonder though if the ideal thing would be to detect if we're being run by npm or yarn (assuming that's even possible) and then do the same for the sub-packages, rather than creating a new version of this library for Yarn? My thinking here is that it's not really the library author's job to say whether the end-developer should use npm or yarn to install with?

dchambers avatar Nov 12 '16 19:11 dchambers

At this point, I'd prefer not to auto detect npm vs yarn since, at least in my workflow, I tend to use both (though it should be possible by checking for the presence of a yarn.lock file). I also with agree with you about not forcing one or the other one developers, which is why I'm proposing architecting this module to be able to use either!

emgeee avatar Nov 12 '16 19:11 emgeee

Ah, so I was thinking of detecting whether the user has just typed npm install or yarn, rather than whether they have once previously used Yarn, or whether somebody else that's committed to the repo has used Yarn, based on the presence of yarn.lock files. But I'm not sure that's even possible -- I took a look at the yarn Github issues and didn't see anything either way.

If you're open to the idea I can take a look on Monday, and see if it's do-able or not?

dchambers avatar Nov 12 '16 20:11 dchambers

I'm not quite sure what you mean by detecting what the user just typed but I think the simple solution would just be to add a yarn-recursive-install script that does the same thing as npm-recursive-install.

emgeee avatar Nov 13 '16 17:11 emgeee

I'd like to see this support yarn as well. This would be really useful for working with serverless.

recursive-test is a good idea as well but maybe we should change the name of the project as well. Maybe recursive-commands perhaps?

dashmug avatar Dec 22 '16 05:12 dashmug