[BUG] npm find-dupes and npm dedupe --dry-run doesn't print anything useful #2687

Activity

1. isaacs

   added

   Release 7.xwork is associated with a specific npm 7 releasework is associated with a specific npm 7 release

   Bugthing that needs fixingthing that needs fixing

   Needs Triageneeds review for next stepsneeds review for next steps

   on Feb 12, 2021
2. darcyclarke

   added

   Good First Issuegood issue or PR for newcomersgood issue or PR for newcomers

   and removed

   Needs Triageneeds review for next stepsneeds review for next steps

   on Mar 12, 2021
3. chowkapow commented on Mar 17, 2021

   

   chowkapow

   on Mar 17, 2021

   Last edited by chowkapow

   Contributor

   More actions

   Hey! I would like to work on this issue.

   How can I test the output for my changes in dedupe.js in local to see if it can match the expected behavior?

   await arb.dedupe(this.npm.flatOptions)
   // await reifyOutput(this.npm, arb);
   await reifyFinish(this.npm, arb);
   
4. chowkapow commented on Mar 19, 2021

   

   chowkapow

   on Mar 19, 2021

   Contributor

   More actions

   Figured out how to test locally with bin/npm-cli.js. 😊

   Now I'm trying to set npm.flatOptions.dryRun === true before calling reify-output() as suggested.
   However, I'm getting errors saying it's not a settable property, e.g. this.npm.flatOptions.dryRun = true.

   Any more tips?
5. isaacs commented on Mar 20, 2021

   

   isaacs

   on Mar 20, 2021

   Last edited by isaacs

   ContributorAuthor

   More actions

   Hey, @chowkapow! Thanks for diving into this UX issue.

   We are in the process of a somewhat massive overhaul of the way that configuration parameters are handled and managed. It may be easiest at the current moment to base your work on the release-next branch for the time being, and then rebase onto latest once the next version is shipped.

   In that branch, you'll see in the lib/find-dupes.js command that it has these lines:

   class FindDupes {
     // ... description, name, eventually a usage() method, etc.
     exec (args, cb) {
       this.npm.config.set('dry-run', true)
       this.npm.commands.dedupe([], cb)
     }
   }

   That's the appropriate way to set configs now. The flatOptions.dryRun flag will be set based on calling npm.config.set('dry-run', true) (so the internal flatOptions and the user-facing config fields are always kept in sync). There will be more cleanup and refactoring in this area, but this should be the consistent pattern moving forward -- internally, npm/cli uses this.npm.config.set() and this.npm.config.get() with the hyphenated names, and all the deps use the camelCase flattened option names.

   As a first pass here, since we also want similar behavior for npm install --dry-run, npm update --dry-run, and so on, I'd suggest adding the appropriate output in the lib/utils/reify-output.js module. reifyOutput takes an npm object as the first parameter, so it could do something like this:

   const reifyOutput = (npm, arb) => {
     // don't print any info in --silent mode
   
     // XXX: maybe it _should_ print something in dry-run anyway?
     // I'm not sure.  Even if it's "silent", isn't the pretty printed diff
     // kind of "the point" when it's a dry-run?  Maybe play with this?
     if (log.levels[log.level] > log.levels.error)
       return
   
     const { diff, actualTree } = arb
   
     // XXX: added bit here, not sure if it goes here?  Should it be
     // later?  Earlier?  Some room for creativity/iteration here.
     if (npm.config.get('dry-run')) {
       // a method that will show what WOULD have been done
       prettyPrintDiff(diff) // might need to pass the npm object, if anything in here is config-specific?
       // maybe return?  Maybe it makes sense to still print the rest?
       // this is where we should try some stuff and see what UX feels right
     }

   That prettyPrintDiff() method is where the new "display the diff nicely" would go. That also can have some creativity and iteration, to see what is enough information, too much, etc. A first pass might be to look at the npm v6 find-dupes output, but we don't have to be too tied to that, if it doesn't make sense.

   Then none of the commands have to change (because any dry-run reification command will just go through this method, which knows what has to be shown to the user).

   Another open question, should find-dupes have the same dry-run output as npm install --dry-run? After all, npm find-dupes is effectively the same as npm install --prefer-dedupe --dry-run.

   Once you add some code in there, you can do node . find-dupes to run the code in your local install. (Or node /path/to/npm/cli find-dupes to run it in some other test project.) You can also make link in the npm cli project to have it take over your "real" globally-installed npm instance, but of course that's more of a commitment.

   I realize I'm answering your question with a bunch more questions, but hopefully it gets you headed in a productive direction ;)
6. isaacs commented on Mar 20, 2021

   

   isaacs

   on Mar 20, 2021

   ContributorAuthor

   More actions

   Once you have the code and output feeling about right, you can write tests in test/lib/utils/reify-output.js to cover all the cases. The existing tests should give you enough clue to get started by copying and modifying them.

   Don't spend too much time trying to make it perfect. Getting something there and then tweaking it with feedback from the CLI team and other users is much more efficient. If it ends up taking a while, better to have something that gets us most of the way there, than have it be blocked. Feel free to send a draft PR with anything you'd like us to take a look at.
7. chowkapow commented on Mar 20, 2021

   

   chowkapow

   on Mar 20, 2021

   Contributor

   More actions

   Hey @isaacs, thank you so much for the thorough explanation! This turned out to have more creative input than I thought.

   I made a PR with some of my initial thoughts and questions. Let me know if this is the right direction and I will continue working.
8. davbrito commented on Apr 2, 2022

   

   davbrito

   on Apr 2, 2022

   Last edited by davbrito

   More actions

   Hi. It seems that this has been around for a while.
   What's the current status of this?
9. rscottlundgren commented on Aug 9, 2022

   

   rscottlundgren

   on Aug 9, 2022

   More actions

   @isaacs - I see that you've done some work on this issue with @chowkapow, but that work was done back on Mar 20, 2021 (most recent comment between the two of you here). Is this issue still relevant/"up-for-grabs"? If so, I'd like to take a whack at it - assuming that it wasn't resolved already by the PR @chowkapow referenced previously. Please let me know.
10. Pulkit0729 commented on Jun 23, 2023

    

    Pulkit0729

    on Jun 23, 2023

    More actions

    Is this still an open issue?
11. Sourav-Kumar-Panda commented on Sep 12, 2023

    

    Sourav-Kumar-Panda

    on Sep 12, 2023

    More actions

    Can i work on this?
12. Blaumaus commented on Jan 13, 2024

    

    Blaumaus

    on Jan 13, 2024

    Contributor

    More actions

    @isaacs I've opened a PR with a fix for this problem: #7133
    I've added functionality to show the difference properly, as well as some tests.
    Please let me know if anything can be improved or added from my side.🙂

    🚀React with 🚀1Reacted by David Brito
13. Tayvon

    closed this as completedin #7133on Jan 17, 2024