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.

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

Add npm-recursive-test command #5

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dchambers
Copy link

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.

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.
@emgeee
Copy link
Owner

emgeee commented Nov 8, 2016

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

@dchambers
Copy link
Author

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?

@emgeee
Copy link
Owner

emgeee commented Nov 11, 2016

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

@dchambers
Copy link
Author

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?

@emgeee
Copy link
Owner

emgeee commented Nov 12, 2016

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!

@dchambers
Copy link
Author

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?

@emgeee
Copy link
Owner

emgeee commented Nov 13, 2016

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.

@dashmug
Copy link

dashmug commented Dec 22, 2016

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants