fix(linker): Fix yarn removing linked deps during link stage #4757

gandazgul · 2017-10-23T01:33:39Z

Summary

Actual fix: changed fs.readlink to fs.realpath when checking if a symlink is a linked dependency in package-linker.js This fixes yarn removing linked deps when installing or updating.

Fixes #3288, fixes #4770, fixes #4635, fixes #4603.

Potential fix for #3202.

Test plan

See #3288 (comment) for repro steps.
See #3288 (comment) for my explanation of the problem.

With a real world test scenario this works, but I'm unable to have it break from a unit test. I added a test in the integration suite but with the bug added back in it still passes because both generated paths are identical. I would like some help with the unit test.

… a linked dependency in package-linker.js Fixes yarn removing linked deps when installing or updating.

buildsize · 2017-10-23T01:44:09Z

This change will increase the build size from 9.94 MB to 9.94 MB, an increase of 935 bytes (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	859.47 KB	859.44 KB	-31 bytes (0%)
yarn-[version].js	3.78 MB	3.78 MB	369 bytes (0%)
yarn-legacy-[version].js	3.83 MB	3.83 MB	586 bytes (0%)
yarn-v[version].tar.gz	865.09 KB	865.17 KB	85 bytes (0%)
yarn_[version]all.deb	654.12 KB	654.05 KB	-74 bytes (0%)

buildsize · 2017-10-23T01:44:09Z

This change will decrease the build size from 9.94 MB to 9.94 MB, a decrease of 277 bytes (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	859.47 KB	859.34 KB	-132 bytes (0%)
yarn-[version].js	3.78 MB	3.78 MB	-181 bytes (0%)
yarn-legacy-[version].js	3.83 MB	3.83 MB	36 bytes (0%)
yarn-v[version].tar.gz	865.09 KB	865 KB	-86 bytes (0%)
yarn_[version]all.deb	654.12 KB	654.2 KB	86 bytes (0%)

gandazgul · 2017-10-23T14:57:56Z

Note to myself, found a bug, yarn throws when a linked module's destination no longer exists, try catch?

Fixed with: f4ed169

Provides a helpful warning and removes the offending module.

bestander · 2017-10-24T04:46:04Z

package.json

@@ -1,7 +1,7 @@
 {
  "name": "yarn",
  "installationMethod": "unknown",
-  "version": "1.2.1",
+  "version": "1.2.2",


this is unnecessary, the core team will bump the patch version when they are ready to release it

I will revert this. Thank you.

bestander · 2017-10-24T04:50:03Z

src/package-linker.js

@@ -304,8 +304,15 @@ export default class PackageLinker {
      const stat = await fs.lstat(entryPath);

      if (stat.isSymbolicLink()) {
-        const packageName = entry;
-        linkTargets.set(packageName, await fs.readlink(entryPath));
+        try {


eslint errors on this.
Please run yarn run prettier to autoformat the code

Will do, and will check with the linter before committing. Thanks.

Done. yarn lint completed.

bestander · 2017-10-24T04:53:28Z

__tests__/commands/install/integration.js

+    {},
+    'install-dont-overwrite-linked',
+    async (config): Promise<void> => {
+      const dependencyPath = path.join(config.cwd, 'node_modules', 'fake-dependency');


I don't understand the test.
i would expect that you would run a link command and then install should not overwrite it?

A few tips.

You probably want to setup several folders for the test: a linked package.json and the linker package.json

here is how link command is used in tests https://github.com/yarnpkg/yarn/blob/master/__tests__/commands/link.js#L35

after linking you can check that link exists

after that run install command and check that link was not destroyed
https://github.com/yarnpkg/yarn/blob/master/__tests__/commands/install/integration.js#L93

Thank you! Yeah I dont get this test either. I copied it from the other one that checks if scoped links are maintained, I suspect that one is faulty too.

I'll update both with your suggestions here.

…ith the existing code Added the try catch for possible missing linked module targets to the scoped logic to mirror the regular packages Simplified the fixture for testing that yarn doesn't remove linked deps Moved runLink to helpers to be able to use it on other test suites

gandazgul · 2017-10-25T01:57:25Z

I re-wrote the tests with @bestander suggestions and tested them with the old code to make sure they break. Both tests for scoped and regular packages now fail correctly and pass correctly with the new code.

…ined and not just falsy.

BYK

See #3288 (comment) for my explanation of the problem.

I think explaining the reason for the change in the commit log would be great for any future readers. I think this is important since there are no comments in the code (and I'd argue comments are not necessary) but the reason for the change is important.

BYK · 2017-10-26T10:02:27Z

__tests__/commands/_helpers.js

-      cwd = path.join(cwd, name.cwd);
+    // if source wasn't set then assume we were given a complete path
+    if (typeof source === 'undefined') {
+      cwd = typeof name !== 'string' ? name.cwd : await fs.makeTempDir();


These nested conditionals are a bit confusing. Do you think we can find a cleaner way to do this?

they are, if I take this one off which is unnecessary then flow complains. I will take a look in another PR see if I can can come up with a clearer logic and put more comments.

BYK · 2017-10-26T10:03:31Z

__tests__/commands/link.js

@@ -1,24 +1,12 @@
 /* @flow */

-import {run as buildRun} from './_helpers.js';
-import {run as link} from '../../src/cli/commands/link.js';
+import {runLink} from './_helpers.js';


Nice refactor.

BYK · 2017-10-26T10:09:28Z

src/package-linker.js

@@ -317,7 +322,13 @@ export default class PackageLinker {

          if (stat2.isSymbolicLink()) {
            const packageName = `${scopeName}/${entry2}`;
-            linkTargets.set(packageName, await fs.readlink(entryPath2));
+            try {


This repeated code kind of calls for a helper function.

Yeah I was thinking about it. But I didn't want to confuse this too much. There's also a todo comment to merge the link targets logic here with logic in another part of the code. I can take a look after and make another PR.

BYK · 2017-10-26T10:13:34Z

LGTM. @bestander approve and merge at will!

BYK · 2017-10-26T10:14:51Z

Fixes: #3288 and other reported issues relating to linked deps being removed.

Do we have a list for these?

gandazgul · 2017-10-26T13:41:06Z

@BYK I found these:

#4770
#4635
#4603

Thanks for approving the PR. :)

bestander · 2017-10-26T15:22:47Z

Great job, @gandazgul!
Thanks for helping with review, @BYK

bertho-zero · 2017-10-26T15:52:07Z

It works perfectly, npm has this problem since the beginning of version 3, thank you very much!

gandazgul · 2017-10-26T21:22:27Z

@bestander thank you, happy to help. When do you think this will be released? My team is anxious to upgrade yarn to the latest version.

…#4757) **Summary** Actual fix: changed fs.readlink to fs.realpath when checking if a symlink is a linked dependency in package-linker.js This fixes yarn removing linked deps when installing or updating. Fixes yarnpkg#3288, fixes yarnpkg#4770, fixes yarnpkg#4635, fixes yarnpkg#4603. Potential fix for yarnpkg#3202. **Test plan** See yarnpkg#3288 (comment) for repro steps. See yarnpkg#3288 (comment) for my explanation of the problem. With a real world test scenario this works, but I'm unable to have it break from a unit test. I added a test in the integration suite but with the bug added back in it still passes because both generated paths are identical. I would like some help with the unit test.

bestander · 2017-10-28T07:07:49Z

@gandazgul it is due to be released any day soon according to recent release history.
In the mean time feel free to use nightly builds https://yarnpkg.com/en/docs/nightly

rdsedmundo · 2018-05-30T13:09:25Z

This PR hasn't fixed this problem entirely. I'm still facing this issue.

NodeJS version: 10.0.0
Yarn version: 1.6.0

I do the yarn link package and I can confirm it works by executing ls -l node_modules/package and seeing the symlink pointing to the right folder. After I run yarn install though and execute the ls -l node_modules/package command again, the symlink is removed and it does use the package from what is on package.json again (it's a Git URL by the way, don't know if it makes difference).

Fix: changed fs.readlink to fs.realpath when checking if a symlink is…

e07acc3

… a linked dependency in package-linker.js Fixes yarn removing linked deps when installing or updating.

gandazgul mentioned this pull request Oct 23, 2017

yarn overwrites symlinked dependencies #3288

Closed

Version bump

f454510

Fixes the error thrown when a linked module's target no longer exists

f4ed169

Provides a helpful warning and removes the offending module.

bestander reviewed Oct 24, 2017

View reviewed changes

gandazgul added 2 commits October 24, 2017 22:00

Forgot to delete old test

adffa41

source might be an empty string sometimes. Have to make sure is undef…

16e21a7

…ined and not just falsy.

BYK approved these changes Oct 26, 2017

View reviewed changes

BYK changed the title ~~Fixed yarn removing linked deps when installing or upgrading~~ fix(linker): Fix yarn removing linked deps during link stage Oct 26, 2017

BYK merged commit 9b0e7bb into yarnpkg:master Oct 26, 2017

BYK mentioned this pull request Oct 26, 2017

yarn upgrade breaks dependencies #3202

Open

This was referenced Oct 26, 2017

npm install after npm link will "steal" dependencies from linked packages npm/npm#10343

Closed

Installing with npm 5 uninstalls dependencies of locally linked module npm/npm#17320

Open

BYK mentioned this pull request Oct 27, 2017

Yarn does not reliably choose installation location of conflicting dependencies #3005

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(linker): Fix yarn removing linked deps during link stage #4757

fix(linker): Fix yarn removing linked deps during link stage #4757

gandazgul commented Oct 23, 2017 •

edited by BYK

Loading

buildsize bot commented Oct 23, 2017 •

edited

Loading

buildsize bot commented Oct 23, 2017

gandazgul commented Oct 23, 2017 •

edited

Loading

bestander Oct 24, 2017

gandazgul Oct 24, 2017

bestander Oct 24, 2017

gandazgul Oct 24, 2017

gandazgul Oct 25, 2017

bestander Oct 24, 2017

bestander Oct 24, 2017

gandazgul Oct 24, 2017

gandazgul commented Oct 25, 2017

BYK left a comment

BYK Oct 26, 2017

gandazgul Oct 26, 2017

BYK Oct 26, 2017

BYK Oct 26, 2017

gandazgul Oct 26, 2017

BYK commented Oct 26, 2017

BYK commented Oct 26, 2017

gandazgul commented Oct 26, 2017 •

edited

Loading

bestander commented Oct 26, 2017

bertho-zero commented Oct 26, 2017

gandazgul commented Oct 26, 2017

bestander commented Oct 28, 2017

rdsedmundo commented May 30, 2018

fix(linker): Fix yarn removing linked deps during link stage #4757

fix(linker): Fix yarn removing linked deps during link stage #4757

Conversation

gandazgul commented Oct 23, 2017 • edited by BYK Loading

buildsize bot commented Oct 23, 2017 • edited Loading

buildsize bot commented Oct 23, 2017

gandazgul commented Oct 23, 2017 • edited Loading

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

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

gandazgul commented Oct 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK commented Oct 26, 2017

BYK commented Oct 26, 2017

gandazgul commented Oct 26, 2017 • edited Loading

bestander commented Oct 26, 2017

bertho-zero commented Oct 26, 2017

gandazgul commented Oct 26, 2017

bestander commented Oct 28, 2017

rdsedmundo commented May 30, 2018

gandazgul commented Oct 23, 2017 •

edited by BYK

Loading

buildsize bot commented Oct 23, 2017 •

edited

Loading

gandazgul commented Oct 23, 2017 •

edited

Loading

gandazgul commented Oct 26, 2017 •

edited

Loading