Fix: Local package dependency resolving (refs #3897) #3945

ghost · 2017-07-18T03:34:30Z

Summary
File protocol dependencies will now be de-duped correctly. E.g. './a', 'a', and '/absolute/path/to/a' will be treated equivalently. Fixes issue #3897.

Test plan
Test added to cover the above cases.

BYK

Thanks a lot for the fix. I've added some comments. My concern is mostly around the tests but if you can factor in my suggestions for the main code, that'd be great!

BYK · 2017-07-18T10:40:06Z

src/lockfile/wrapper.js

@@ -184,6 +186,8 @@ export default class Lockfile {

      if (remote.resolved) {
        seen.set(remote.resolved, obj);
+      } else if (remote.type == 'copy' && remote.reference && remote.hash) {


May be you need a helper:

const keyForRemote = remote => remote.reference && remote.hash ? `${remote.reference}#${remote.hash}` : null;

BYK · 2017-07-18T10:41:49Z

src/package-resolver.js

@@ -388,14 +388,18 @@ export default class PackageResolver {
      }
    }

+    if (getExoticResolver(version) && manifest) {


May be check manifest first to avoid the function call when it is missing?

BYK · 2017-07-18T10:42:51Z

src/package-resolver.js

@@ -388,14 +388,18 @@ export default class PackageResolver {
      }
    }

+    if (getExoticResolver(version) && manifest) {
+      this.exoticRangeMatch(patterns.map(p => this.getStrictResolvedPattern(p)), manifest);


You can do patterns.map(this.getStrictResolvedPattern.bind(this)), manifest); instead.

BYK · 2017-07-18T10:43:12Z

src/package-resolver.js

@@ -410,7 +414,10 @@ export default class PackageResolver {
    });

    const maxValidRange = semver.maxSatisfying(versionNumbers, range);
-    if (!maxValidRange) {
+
+    if (!maxValidRange && getExoticResolver(range) && manifest) {


Same as above, check manifest first?

BYK · 2017-07-18T10:45:44Z

src/package-resolver.js

+
+    if (!maxValidRange && getExoticResolver(range) && manifest) {
+      return this.exoticRangeMatch(resolvedPatterns, manifest);
+    } else if (!maxValidRange) {


Looks like this part needs some refactoring:

if (!maxValidRange) { return manifest && getExoticResolver(range) ? this.exoticRangeMatch(resolvedPatterns, manifest) : null; }

BYK · 2017-07-18T10:48:50Z

src/package-resolver.js

+  exoticRangeMatch(resolvedPkgs: Iterable<Manifest>, manifest: Manifest): ?Manifest {
+    if (!manifest._remote) {
+      return null;
+    } else if (manifest._remote.type != 'copy' || !manifest._remote.reference) {


You return null in both cases so why not:

if (!(manifest._remote && manifest._remote.reference && manifest._remote.type === 'copy')) { return null; }

This also makes your intent clearer.

BYK · 2017-07-18T10:53:43Z

src/package-resolver.js

+      return null;
+    }
+
+    for (const pkg of resolvedPkgs) {


const matchedPkg = resolvedPkgs.find(pkg => { const remote = pkg._remote; return remote && remote.reference.manifest._remote.reference && remote.type === 'copy'); }); if (matchedPkg) { manifest._remote = match._remote; } return matchedPkg;

BYK · 2017-07-18T10:56:45Z

__tests__/commands/install/integration.js

@@ -437,6 +437,24 @@ test.concurrent('install with file: protocol as default', (): Promise<void> => {
  });
 });

+test.concurrent('install file: dedupe dependencies', async (): Promise<void> => {
+  await runInstall({}, 'install-file-dedupe-dependencies', async (config, reporter) => {
+    expect(await fs.exists(path.join(config.cwd, 'node_modules', 'a', 'node_modules'))).toEqual(false);


Add a comment above this stating what you are testing or be more explicit with your test, please. I'm guessing this is to make sure b is not added as a sub-dependency of a, right?

BYK · 2017-07-18T10:58:25Z

__tests__/commands/install/integration.js

+    const pkgJsonContents = await fs.readFile(path.join(config.cwd, 'c/package.json'));
+    const pkgJson = JSON.parse(pkgJsonContents);
+    pkgJson.dependencies = {b: `file:${path.join(config.cwd, 'b')}`};
+    await fs.writeFile(path.join(config.cwd, 'c/package.json'), JSON.stringify(pkgJson));


Why are we doing this? Feels like this needs to be a separate test case or handled via a yarn add b@file:... command instead of directly manipulating the package.json file in the fixture.

BYK · 2017-07-18T11:00:01Z

__tests__/commands/install/integration.js

+    const reInstall = new Install({}, config, reporter, lockfile);
+    await reInstall.init();
+
+    expect(await fs.exists(path.join(config.cwd, 'node_modules', 'c', 'node_modules'))).toEqual(false);


Again, what are we testing? I'm guessing you are testing now c also doesn't have any sub-dependencies even if we added b to it. I'd suggest creating a separate test case for this whole scenario. It would make your test cases more understandable and atomic so if something goes wrong in the future, we won't look at this test and try to decipher what exactly went wrong :)

ghost · 2017-07-18T17:15:30Z

Thanks for the review. I'll work on those changes.

ghost · 2017-07-21T19:50:19Z

Code refactored and test split into two separate cases with better comments.

BYK

Nice!

I'll wait for you to respond to my inline comments and then I'll merge with or without changes per your preference.

Thanks for sticking to this!

BYK · 2017-07-25T12:43:20Z

src/lockfile/wrapper.js

@@ -156,7 +160,8 @@ export default class Lockfile {
      invariant(ref, 'Package is missing a reference');
      invariant(remote, 'Package is missing a remote');

-      const seenPattern = remote.resolved && seen.get(remote.resolved);
+      const altRemoteKey = keyForRemote(remote);


I think we should merge all this functionality into keyForRemote and just use it everywhere without the remote.resolved checks. What do you think?

Yes this is better. It looks like there may be one case where a remote may have a resolved prop but a falsy hash prop, so I added a check for remote.resolved in keyForRemote.

BYK · 2017-07-25T12:45:26Z

src/package-resolver.js

+    }
+
+    const matchedPkg = resolvedPkgs.find(({_remote: pkgRemote}) => {
+      return pkgRemote && pkgRemote.reference === remote.reference && pkgRemote.type === 'copy';


No need for return:

resolvedPkgs.find(({_remote: pkgRemote}) => pkgRemote && pkgRemote.reference === remote.reference && pkgRemote.type === 'copy');

This one exceeded the max line length lint rule, so I left it as is for now.

Prettier should fix it automatically (yarn prettier). Or you can do it manually:

resolvedPkgs.find( ({_remote: pkgRemote}) => pkgRemote && pkgRemote.reference === remote.reference && pkgRemote.type === 'copy' );

BYK · 2017-07-25T12:47:16Z

__tests__/commands/install/integration.js

+test.concurrent('install file: dedupe dependencies 2', (): Promise<void> => {
+  return runInstall({}, 'install-file-dedupe-dependencies-2', async (config, reporter) => {
+    // Add b as a dependency, using an absolute path
+    await add(config, reporter, {}, [`b@file:${path.join(config.cwd, 'b')}`]);


fs.realpath might be better than using path.join with config.cwd since it is guaranteed to be absolute where as config.cwd can still be a relative path. Unlikely, but just pointing it out.

Ended up using path.resolve for this since it allows multiple paths to be passed as arguments. Should give the same functionality as fs.realpath.

BYK

Final rejection I promise. It's about the concern around that potentially missing return statement.

BYK · 2017-07-26T09:41:13Z

src/package-resolver.js

@@ -388,14 +388,18 @@ export default class PackageResolver {
      }
    }

+    if (manifest && getExoticResolver(version)) {
+      this.exoticRangeMatch(patterns.map(this.getStrictResolvedPattern.bind(this)), manifest);


Did you mean to return this? Otherwise I'm a bit confused since we return null right after this.

Yep, good catch. Updated.

Well, this means the tests don't cover this line :( we should add a test case in a follow-up.

Yes, I'll look into why the added tests don't seem to be covering this.

File protocol dependencies will now be deduped correctly. E.g. './a', 'a', and '/absolute/path/to/a' will all be deduped.

BYK · 2017-07-26T17:25:15Z

LGTM, will merge once Travis passes.

**Summary** Adding an additional test for an uncovered method from pull request #3945. The previous tests did not cover this, since the method, `getExactVersionMatch`, is only called if a package's `fresh` prop is false or the frozen flag is set. The fixtures directory for this test contains a lockfile to meet this condition. If the `manifest && getExoticResolver(version)` condition does not evaluate properly the `multiplePackagesCantUnpackInSameDestination` warning is logged. This test checks for that. **Test plan** New tests, obviously :)

ghost changed the title ~~Bugfix #3897: Fixing local package dependency resolving~~ Bugfix 3897: Fixing local package dependency resolving Jul 18, 2017

BYK requested changes Jul 18, 2017

View reviewed changes

ghost changed the title ~~Bugfix 3897: Fixing local package dependency resolving~~ Fix: Local package dependency resolving (refs #3897) Jul 21, 2017

BYK approved these changes Jul 25, 2017

View reviewed changes

BYK requested changes Jul 26, 2017

View reviewed changes

Fixing local package dependency resolving

63ebcd5

File protocol dependencies will now be deduped correctly. E.g. './a', 'a', and '/absolute/path/to/a' will all be deduped.

BYK approved these changes Jul 26, 2017

View reviewed changes

BYK merged commit 6d793ae into yarnpkg:master Jul 26, 2017

BYK self-assigned this Jul 26, 2017

ghost deleted the local-pkg-resolving branch August 2, 2017 03:38

ghost mentioned this pull request Aug 2, 2017

Adding additional test for pull request #3945 #4078

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Local package dependency resolving (refs #3897) #3945

Fix: Local package dependency resolving (refs #3897) #3945

ghost commented Jul 18, 2017 •

edited by ghost

Loading

BYK left a comment

BYK Jul 18, 2017

BYK Jul 18, 2017

BYK Jul 18, 2017

BYK Jul 18, 2017

BYK Jul 18, 2017

BYK Jul 18, 2017

BYK Jul 18, 2017

BYK Jul 18, 2017

BYK Jul 18, 2017

BYK Jul 18, 2017

ghost commented Jul 18, 2017

ghost commented Jul 21, 2017

BYK left a comment

BYK Jul 25, 2017

ghost Jul 25, 2017 •

edited by ghost

Loading

BYK Jul 25, 2017

ghost Jul 25, 2017

BYK Jul 26, 2017 •

edited

Loading

BYK Jul 25, 2017

ghost Jul 25, 2017

BYK Jul 26, 2017

BYK left a comment

BYK Jul 26, 2017

ghost Jul 26, 2017

BYK Jul 26, 2017

ghost Jul 26, 2017

BYK commented Jul 26, 2017

Fix: Local package dependency resolving (refs #3897) #3945

Fix: Local package dependency resolving (refs #3897) #3945

Conversation

ghost commented Jul 18, 2017 • edited by ghost Loading

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

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

ghost commented Jul 18, 2017

ghost commented Jul 21, 2017

BYK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 25, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BYK Jul 26, 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

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

BYK commented Jul 26, 2017

ghost commented Jul 18, 2017 •

edited by ghost

Loading

ghost Jul 25, 2017 •

edited by ghost

Loading

BYK Jul 26, 2017 •

edited

Loading