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

Please don't log every npm dependency which doesn't have the ropm keyword #39

Open
elsassph opened this issue May 26, 2021 · 6 comments

Comments

@elsassph
Copy link

elsassph commented May 26, 2021

My project heavily uses npm modules and ropm copy is uber verbose:

ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/color-support" because it does not have the "ropm" keyword
ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/tap-mocha-reporter" because it does not have the "ropm" keyword
ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/tap-mocha-reporter/node_modules/debug" because it does not have the "ropm" keyword
ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/tap-mocha-reporter/node_modules/ms" because it does not have the "ropm" keyword
ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/tap-mocha-reporter/node_modules/escape-string-regexp" because it does not have the "ropm" keyword
ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/tap-parser" because it does not have the "ropm" keyword
ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/diff" because it does not have the "ropm" keyword
ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/tap-yaml" because it does not have the "ropm" keyword
ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/minipass" because it does not have the "ropm" keyword
ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/events-to-array" because it does not have the "ropm" keyword
ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/yaml" because it does not have the "ropm" keyword
ropm: skipping prod dependency "/Users/philippe.elsass/dev/my-project/node_modules/unicode-length/node_modules/strip-ansi" because it does not have the "ropm" keyword
[... a hundred more lines of that]

That message REALLY isn't useful.

@TwitchBronBron
Copy link
Member

So I do realize it's verbose, but many ropm developers (and publishers) aren't overly experienced with npm/ropm things, so it's helpful in my opinion to explain why a certain package didn't get copied to your rootDir.

That being said, perhaps we can introduce a logLevel flag, where the default would emit a single line:

143 prod dependencies were not copied because they do not have the `ropm` keyword

and then the more verbose loglevel would be where the full list of skipped packages would be printed.

@elsassph
Copy link
Author

But what is it useful for?
As a ropm user you will install various modules, usually following the instructions.
Then you run ropm copy and you see which modules are copied, which is the useful information.

@TwitchBronBron
Copy link
Member

TwitchBronBron commented May 26, 2021

What about this scenario?

ropm i lodash

Lodash is definitely not a ropm module. but ropm will "install it" just fine because ropm just initiates npm install behind the scenes. Also, ropm i runs ropm copy as well.

Now, when I run ropm copy, this is the only output I get right now:

C:\projects\RokuProject>npx ropm copy
ropm: skipping prod dependency "C:\projects\RokuProject\node_modules\lodash" because it does not have the "ropm" keyword

C:\projects\RokuProject>

I would much rather warn users about something not looking right, rather than making them investigate it themselves ("and you see which modules are copied") every time they run install.

@elsassph
Copy link
Author

I'd suggest adding some ropm list maybe. But having this log on every ropm copy for every transitive dependency is a lot of noise.

@georgejecook
Copy link
Contributor

I'm not a fan of the noise either, as it goes.
I think this is a great solution:

That being said, perhaps we can introduce a logLevel flag, where the default would emit a single line:

143 prod dependencies were not copied because they do not have the `ropm` keyword
and then the more verbose loglevel would be where the full list of skipped packages would be printed.

@pawelhertman
Copy link
Contributor

pawelhertman commented Aug 6, 2021

What about this scenario?

ropm i lodash

Lodash is definitely not a ropm module. but ropm will "install it" just fine because ropm just initiates npm install behind the scenes. Also, ropm i runs ropm copy as well.

Maybe that's the problem - maybe ROPM should inform that the package user is trying to install has no ropm keyword and will be ignored?
Therefore even

143 prod dependencies were not copied because they do not have the ropm keyword

won't be necessary - what user should do with such information? I think it's useless

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

No branches or pull requests

4 participants