Fixed issue #1123 extra color for bolded text in the output

[a-amaria]

Summary

This pull request presents a solution for issue #1123, adding the suggested color for the bolded input.

Motivation

This issue was picked because as we didn't have much experience, this seemed like a good first issue so we could have some entry way point.
This isn't a substantial bug fix or feature request, it merely adds the convenience of distinguishing better which dependencies were updated, by bolding them and giving them a different color in the tree that shows up after yarn add is used.

Test plan

We tested the developed code by installing different versions of "lodash" by doing yarn add [email protected] for an exemple, but we kept changing the versions so we could see the tree with the altered dependencies. We also tested with another package, standard-version yarn add [email protected], following a similar procedure. We were only able to test on Windows so far.

The result can be seen in the following images

EDIT: By distraction I had created a pull request in my own fork of the repo, however as I noticed it I fixed it by creating this one.

[buildsize]

This change will decrease the build size from 10.26 MB to 10.25 MB, a decrease of 4.12 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	886.52 KB	886.14 KB	-385 bytes (0%)
yarn-[version].js	3.86 MB	3.86 MB	-1.62 KB (0%)
yarn-legacy-[version].js	4.01 MB	4.01 MB	-1.61 KB (0%)
yarn-v[version].tar.gz	891.28 KB	890.96 KB	-325 bytes (0%)
yarn_[version]all.deb	668.08 KB	667.88 KB	-202 bytes (0%)

[a-amaria]

It seems to me that some of the tests are failing because the tests are prepared for a different color (null), and so the snapshot doesn't match the received, as seen here, even though the trees are virtually the same except for the intended new feature of having colored and bolded dependencies.

[bestander]

@Koreris, nice first PR!
Thanks for sending it in

[bestander]

@Koreris, can you update the snapshots ./node_modules/.bin/jest -u?

[bestander]

This seems fine to me, I don't have a strong opinion on the bold/not bold usage in the formatter.
@olingern, you have been looking into this feature quite a lot, what do you think?

[a-amaria]

@bestander I have been looking for that file but maybe I'm looking in the wrong folder? Because I don't seem to find it

[bestander]

Maybe it is OS specific.

Try
node node_modules/jest/bin/jest.js -u

[ClaudiaRaquelGuedes]

@bestander Hi! Me and @Koreris found the file but don't know exactly what to do with it.

[bestander]

You need to run command node node_modules/jest/bin/jest.js -u from CMD when you cd to yarn folder.

[rally25rs]

Hi @Koreris thanks for the contribution!

I'd like to ask you to move the color change to a different spot in the code. Having it in console-reporter causes all output using the tree() function to be in cyan. If you do a global search for .tree( you can find it used a couple other places. For example the commands yarn list and yarn licenses list are now all cyan.

A bit of a hint is that in this code block:

    const output = ({name, children, hint, color}, titlePrefix, childrenPrefix) => {
      const formatter = this.format.bold;
      const out = getFormattedOutput({
        prefix: titlePrefix,
        hint,
        color: 'cyan',
        name,
        formatter,
      });
      this.stdout.write(out);

      if (children && children.length) {
        recurseTree(sortTrees(children), childrenPrefix, output);
      }
    };

the color variable is no longer used. I'm actually really surprised that Flow or ESLint doesn't throw an error on that. 😕

Anyway, if you backtrack through the code a bit, you can find that the add command builds its info to send to the ConsoleReporter.tree() function over in src/cli/commands/list.js buildTree()

    let color = 'bold';

    ...

    if (info.originalKey !== info.key || opts.reqDepth === 0) {
      // was hoisted
      color = null;
    }

    ...

    if (topLevel || nextDepthIsValid || showAll) {
      treesByKey[info.key] = {
        name: `${info.pkg.name}@${info.pkg.version}`,
        children,
        hint,
        color,
        depth,
      };
    }

    ...

        if (!hoistedByKey[`${info.key}#${pkg.name}`] && (nextChildDepthIsValid || showAll)) {
          children.push({
            name: pattern,
            color: 'dim',
            shadow: true,
          });
        }

So bold, dim and "normal" (null) are all used here. The original issue looks like they wanted some color added to "bold" so I think you could just change let color = 'bold'; to let color = 'cyan';

---

There seems to be a bigger issue in what the output shows when doing a yarn add; it looks like it is only showing the top level and hoisted dependencies, which is a bit weird. This check:

    if (info.originalKey !== info.key || opts.reqDepth === 0) {
      // was hoisted
      color = null;
    }

is causing all the output for yarn add to be normal text because add passes reqDepth=0 to this function.

yarn list does not, and so shows all 3 colors (bold, normal, dim). But I think that may be a bigger issue that you can ignore for this PR.

---

For the unit tests, look in __tests__/commands/__snapshots__/list.js.snap and you can see what objects the test is expecting. It has the color in it:

        Object {
          "children": Array [],
          "color": "bold",   <-- here
          "depth": 0,
          "hint": null,
          "name": "[email protected]",
        },

so when it was changed to 'cyan' these comparisons no longer match. You would just need to change the color in the snapshot file to match the changed color.

You can run yarn run test to run the tests locally so you can see if they pass before pushing commits.

Hope this helps! Thanks again!

[a-amaria]

@rally25rs Thanks for the feedback, it's much appreciated! I'll work on it as soon as I can and fix those problems.

[rally25rs]

It also looks like somewhere along the way we got a package-lock.json file committed, which should not exist. That would probably be from an npm install but we should be using yarn not npm ;)

I also think the commits that merged then reverted the merge with master are is making the diff look weird now. It should be fine to have master merged into this branch.

[olingern]

Late to the party, sorry about that.

@bestander

This seems fine to me, I don't have a strong opinion on the bold/not bold usage in the formatter.
@olingern, you have been looking into this feature quite a lot, what do you think?

edit: went back and looked at previous discussion again. I think this works. 👍

@Koreris If someone disables terminal colors, I imagine it stays consistent and outputs as normal? i.e. no bolded text, just plain tree output

Lastly -- is there a preference of cyan over green? I know Yarn's logo is in the cyan neighborhood, but is that a good signifier for this use case? Trivial, I know, but I think it merits some thought.

[olingern]

+1 to @rally25rs comment. We'll need to remove the package-lock.json and rebase master over this branch. Should make review easier

[arcanis]

I'm going to close this PR for now to have a better overview of the ones currently active, but feel free to reopen it once you've been able to work on @rally25rs's comment! And thanks for your involvement into making Yarn even better 😄