Fix duplication of available bin commands (#1991)

[DenisGorbachev]

Summary

This PR solves #1991.

Test plan

I've modified __tests__/commands/run.js to reflect the fixed behavior.

Also, I ran all test suites:

Test Suites: 1 skipped, 36 passed, 36 of 37 total
Tests:       18 skipped, 279 passed, 297 total
Snapshots:   50 passed, 50 total
Time:        28.826s
Ran all test suites.

[hpurmann]

I think I'd rather de-duplicate the bin folder paths of the available registries beforehand. This keeps the concerns better separated than doing this inside a for-loop. You could even move this into a small, well-named function then :)

[DenisGorbachev]

I'd love to - however, there's no duplication of the paths inside each registry, so this check has to be done in a for-loop anyway. Or maybe I misunderstand the suggestion?

[hpurmann]

Let me phrase it differently: I'd do a small loop to construct an array of paths which could be de-duplicated with something like uniq before running the actual loop. This would be one way to solve it in a slightly more functional programming approach (which I think yarn is using way too few).

[DenisGorbachev]

Like this? a968fd0

[hpurmann]

Yeah, I think this is better. You have to be careful though: lodash is not part of yarns dependencies (so it currently only works coincidentally). I'd add the package lodash.uniq separately to yarn.

I think moving the

for (const registry of Object.keys(registries)) {
...
}

loop into a well-named function which takes registries and gives back unique binFolders makes it a bit easier to reason about. I'm not the only one though, other opinions might differ 😉

[torifat]

@hpurmann I think previous approach was simple & clean.

[DenisGorbachev]

@hpurmann @torifat how about applying the YAGNI principle and accepting the first approach (simple one)?

[hpurmann]

Sure, I'm fine with that too. I just think that the overall codebase could profit from a more functional style. But yeah, consistency-wise, the first solution fits perfectly ;)

[torifat]

@hpurmann How using two loops instead of one is more functional? Or, I'm missing something here?

[torifat]

I think here instead of Array we could have just use Set.

[DenisGorbachev]

So let's keep it simple :) I've reverted the code back to original version. Could you please accept the PR?

[torifat]

use const instead of let.

And, please run yarn lint before submitting a PR 😄

[torifat]

@DenisGorbachev Sorry to bother you for one last time. Can you please just use Set. Like in https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/why.js#L113 It would be performant than checking with includes. Thanks for your patience ❤️

[DenisGorbachev]

@torifat sure, changed to Set - let's make yarn faster! 🚀

[torifat]

Thanks for your contribution 😄

[DenisGorbachev]

@torifat @hpurmann Thank you for feedback & merge :)

[hpurmann]

@torifat Even though this is merged already and discussions about programming styles might lead nowhere, I still want to explain my comment and answer yours.

How using two loops instead of one is more functional? Or, I'm missing something here?

Ok, functional programming might be a bit far-fetched. I want to keep concerns separated. I want to call well-named pure functions instead of mutating and accessing the same data structure on different lines. To give an example:

let visited = []
for (const registry of registries) {
  let path = getPath(registry)
  // Do stuff here
  if (!visited.contains(path)) {
    // Do stuff here
    visited.push(entry)
  }
}

There are three lines where we access visited. We added these, and the addition is simple. But when reading it afterwards, I have to keep track of this collection and what it could mean to the execution.

In constrast, the example below uses a function to first collect and return the registry entries (which would probably be using a loop internally). Afterwards, we remove the duplicates.

const availablePaths = getPathsfromRegistries(registries)
const uniquePaths = uniq(availablePaths)

uniquePaths.map((path) => {
  // Do stuff here
})

or even easier, by using flow:

const uniquePaths = flow(
  getPaths,
  uniq
)(registries)

When looking at the actual example, there is no need to iterate over registries anyway. It's only used to construct the paths.

[torifat]

@hpurmann I'm an advocate of this pattern. And, I use things like ramda, ramda-fantasy, data.task a lot. You can check this out: https://github.com/torifat/yarn-update/blob/master/index.js#L15-L34

Yeah, I think this is better. You have to be careful though: lodash is not part of yarns dependencies (so it currently only works coincidentally). I'd add the package lodash.uniq separately to yarn.

The example you have given in your last comment and the change (a968fd0) you said better are not same.

I also prefer using lots of small functions instead of nested loops and if/else. Check this: https://github.com/yarnpkg/yarn/blob/master/src/cli/commands/upgrade-interactive.js#L56-L116

I hope you get my point.