Prioritize bin links at the root level #3877

kaylie-alexa · 2017-07-10T00:29:38Z

Summary
This is a fix for #3801
It seems like there's some history behind this issue, namely

#3310, which 1) looked through all transient dependencies, 2) linked them to node_modules/.bin, then 3) overrode them with any top level dependencies

#3545, removed the last part from above, which I assume was due to it incorrectly detecting what is actually at top level.

So now, the flat tree from PackageHoister gives us a list of all the bin paths that are hoisted to top level, but will include same names if their version ranges don't match, since hoisted has a different meaning from being added as an actual top level dependency. The latest implementation before this PR relied on the whatever order it came from and satisfied the bin link with the first one it found.
So solution was to still allow for the override to happen when the package has isDirectRequired equal to true.

@bestander @rally25rs feel free to jump in with any additional knowledge :)

Test plan
Added tests to actually read versions of the bin links. It passes all the previous test cases with eslint, and passes the new test with uglifyjs bin that fails on master. (Got lucky that we already had uglifyjs tarball in the test fixtures!)

BEFORE

AFTER

Eslint behavior is the same as before

arcanis · 2017-07-10T06:46:07Z

src/package-linker.js

@@ -380,15 +379,15 @@ export default class PackageLinker {
    }
  }

-  determineTopLevelBinLinks(flatTree: HoistManifestTuples): HoistManifestTuples {
+  determineTopLevelBinLinks(flatTree: HoistManifestTuples): Array<[string, Manifest]> {


Does using HoistManifestTuples break something?

No but this function was only used once below, where it just pulled out the Manifest (pkg), and I thought it'd be better to reduce the size of object being passed from the HoistManifest. If you think we need to keep the previous signature, happy to send a refactor PR.

arcanis · 2017-07-10T09:00:55Z

I've merged this PR since it would be nice to ship it along with the 0.28.1 :)

* fix links to only search at root level * remove outdated commment

BYK

This is amazing, thank you!

BYK · 2017-07-10T10:09:57Z

__tests__/commands/install/bin-links.js

@@ -20,6 +21,36 @@ async function linkAt(config, ...relativePath): Promise<string> {
  }
 }

+function execCommand(cwd: string, binPath: Array<string>, args: Array<string>): Promise<Array<?string>> {


I remember seeing a function like this in somewhere else. May be we should start using something like https://yarnpkg.com/en/package/jsinspect to help catch these automatically? (also it would help me be sure instead of saying "I suspect" :D)

Yeah I copied it over from the test file that was testing run command and tweaked it for my purpose. I think it was out of scope for this diff but the right thing to do is probably have a place to store and dedupe all of these similar functions (you're right--I've definitely seen it being reused in several other test files!) inside some shared test utils folder.

On a separate note, that looks like a cool tool! I think I've seen similar functions under src/ which is probably more important to refactor :)

BYK · 2017-07-10T10:11:11Z

__tests__/package-hoister.js

@@ -106,7 +106,7 @@ test('Produces valid destination paths for scoped modules', () => {
    _reference: (({}: any): PackageReference),
  }: any): Manifest);

-  const info = new HoistManifest(key, parts, pkg, '', true, false);
+  const info = new HoistManifest(key, parts, pkg, '', true, true, false);


We really should convert this method to accept and options object instead of boolean arguments. No idea what's happening here :(

rally25rs · 2017-07-10T10:36:30Z

Oh man, this .bin link thing has been such a pain... So the bin linking behavior in <=0.24 was to only link the top-level bins.

In 3535 (which went out as part of v0.25) I started linking the transient deps, but didn't want to change too much code and dig myself a hole I couldn't get out of, so based it on the order of the flat-hoiseted tree, which is at least a deterministic ordering.

However I also left the old/existing logic in place that had already been linking just the direct dependencies, so that no matter what dep was first in the flat-hoist list, which could be a transient dep, it would get overridden by the direct dep, just like <=v0.24 had linked.

3332 removed that last part, so would certainly result in transient deps being .bin linked instead of direct-deps. I think that might be a naming issue on my part that caused some confusion. The function determineTopLevelBinLinks() is used to "determine which transient deps should be linked at the top level (/node_modules/.bin)", not to determine the top-level modules.

I hadn't seen 3332 when it got merged, but it probably indicates that <=v0.24 was linking wrong bins too, since the code that got deleted there was the original (before I messed with it) code that did the bin links for the direct dependencies.

I hope you're attempt to fix these bin links has better luck than mine...

kaylie-alexa

sorry to get to the comments after PR closed!

kaylie-alexa · 2017-07-11T06:13:58Z

__tests__/commands/install/bin-links.js

@@ -20,6 +21,36 @@ async function linkAt(config, ...relativePath): Promise<string> {
  }
 }

+function execCommand(cwd: string, binPath: Array<string>, args: Array<string>): Promise<Array<?string>> {


Yeah I copied it over from the test file that was testing run command and tweaked it for my purpose. I think it was out of scope for this diff but the right thing to do is probably have a place to store and dedupe all of these similar functions (you're right--I've definitely seen it being reused in several other test files!) inside some shared test utils folder.

On a separate note, that looks like a cool tool! I think I've seen similar functions under src/ which is probably more important to refactor :)

kaylie-alexa · 2017-07-11T06:17:01Z

src/package-hoister.js

-      //
-      for (const info of infos) {
-        this.hoist(info);
-      }


fyi, this was unrelated, (so is the for loop below) but the second iteration really didn't seem necessary. lmk if they're need for some reason.

kaylie-alexa · 2017-07-11T06:22:16Z

src/package-linker.js

@@ -380,15 +379,15 @@ export default class PackageLinker {
    }
  }

-  determineTopLevelBinLinks(flatTree: HoistManifestTuples): HoistManifestTuples {
+  determineTopLevelBinLinks(flatTree: HoistManifestTuples): Array<[string, Manifest]> {


No but this function was only used once below, where it just pulled out the Manifest (pkg), and I thought it'd be better to reduce the size of object being passed from the HoistManifest. If you think we need to keep the previous signature, happy to send a refactor PR.

BYK · 2017-07-11T15:14:07Z

sorry to get to the comments after PR closed!

No worries at all :)

Kaylie Kwon added 2 commits July 9, 2017 16:19

fix links to only search at root level

9bd28b3

remove outdated commment

4f1b09c

kaylie-alexa mentioned this pull request Jul 10, 2017

Yarn installs old versions in .bin #3801

Closed

arcanis reviewed Jul 10, 2017

View reviewed changes

arcanis assigned arcanis and kaylie-alexa Jul 10, 2017

arcanis merged commit 3c9b51e into yarnpkg:master Jul 10, 2017

arcanis pushed a commit that referenced this pull request Jul 10, 2017

Prioritize bin links at the root level (#3877)

afeefda

* fix links to only search at root level * remove outdated commment

BYK reviewed Jul 10, 2017

View reviewed changes

kaylie-alexa commented Jul 11, 2017

View reviewed changes

kaylie-alexa deleted the fix-bin-links branch July 11, 2017 06:25

BYK mentioned this pull request Jul 11, 2017

Chore: add code duplication detection tool to lint step #3903

Closed

julienw mentioned this pull request Jul 13, 2017

yarn install doesn't seem to install the mkdirp binary on Linux firefox-devtools/profiler#439

Closed

rally25rs mentioned this pull request Jul 21, 2017

node_modules/.bin are overriden by wrong symlink binaries from not related packages #3983

Closed

deepsweet added a commit to start-runner/webpack that referenced this pull request Jul 22, 2017

👾 freeze Yarn version for CI because of yarnpkg/yarn#3877

Unverified

This user has not yet uploaded their public signing key.

GPG key ID: CDF85E04520152D1

Learn about vigilant mode

b2377e1

This was referenced Jul 27, 2017

regression: yarn fails to install the right version in node_modules/.bin #3837

Closed

docs(aio) - yarn post-install failure due to --source-map not being provided in package.json angular/angular#18488

Closed

edmorley mentioned this pull request Oct 20, 2017

yarn install installs wrong version #4721

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prioritize bin links at the root level #3877

Prioritize bin links at the root level #3877

kaylie-alexa commented Jul 10, 2017

arcanis Jul 10, 2017

kaylie-alexa Jul 11, 2017

arcanis commented Jul 10, 2017

BYK left a comment

BYK Jul 10, 2017

kaylie-alexa Jul 11, 2017

BYK Jul 10, 2017

rally25rs commented Jul 10, 2017 •

edited

Loading

kaylie-alexa left a comment

kaylie-alexa Jul 11, 2017

kaylie-alexa Jul 11, 2017

kaylie-alexa Jul 11, 2017

BYK commented Jul 11, 2017

Prioritize bin links at the root level #3877

Prioritize bin links at the root level #3877

Conversation

kaylie-alexa commented Jul 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcanis commented Jul 10, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rally25rs commented Jul 10, 2017 • edited Loading

kaylie-alexa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK commented Jul 11, 2017

rally25rs commented Jul 10, 2017 •

edited

Loading