Repository: ember-cli/ember-cli
Pull request #6955 (2017-04-12 21:38:19)

Discover dependencies of npm-linked addons
This alters our addon resolution strategy to match node's behavior. Node always resolves symlinks.

This fixes the case where you have an npm-linked dependency with its own dependencies. Today those grandchild dependencies are silently dropped.

I can do a test too. Does it make sense to extend nested-addons-smoke-test-slow so it includes a symlinked addon?

stefanpenner: LGTM

> I can do a test too.

Ya, we should get a test.

> Does it make sense to extend nested-addons-smoke-test-slow so it includes a symlinked addon?

My preference would be to unit test this function, rather then a larger acceptance test.
rwjblue: Is there a practical scenario where what we are doing now fails? Can you give me some steps to try locally to understand the problem you are trying to solve?
stefanpenner: @ef4 should we actually realpath sooner, that way the debug logging is also consistent? Maybe here ? https://github.com/ember-cli/ember-cli/pull/6955/files#diff-6a115371fcdc66c67f9710569fa7c15eR204
stefanpenner: @ef4 is the drop happening here?
https://github.com/ember-cli/ember-cli/blob/28100bbdc1222778072c9148a3693601522a60d4/lib/models/addon.js#L1582-L1588
ef4: @stefanpenner I originally put it there, but that is outside the existsSync guard, so it blows up if the path doesn't exist. 
ef4: @rwjblue yes, there are practical failures.  Scenario:

1. You are developing addonA and you have it npm linked into an app.
2. In addonA you `ember install  -S addonB`.
3. You rebuild the app, but addonB is missing.

stefanpenner: > My preference would be to unit test this function, rather then a larger acceptance test. That is unless, appending it to that existing test reasonably simple.

@ef4 got any cycles for ^ ?
ef4: Yes, working on this.

I think I can do an end to end test that proves the important semantics
(that grandchild modules make it into the build) by extending one of the
existing scenarios (no additional builds that would make the tests slower).

On Mon, Apr 17, 2017 at 6:40 PM Stefan Penner <notifications@github.com>
wrote:

> My preference would be to unit test this function, rather then a larger
> acceptance test. That is unless, appending it to that existing test
> reasonably simple.
>
> @ef4 <https://github.com/ef4> got any cycles for ^ ?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/ember-cli/ember-cli/pull/6955#issuecomment-294616722>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AATfMmRWbs5uGvBbBZJNdTS8mCjAj-Lbks5rw-p3gaJpZM4M8AKJ>
> .
>

ef4: This is turning out to be complicated. I am getting entangled in the PackageCache (I can't readily tweak the contents of node_modules in one acceptance test without leaking that change).
stefanpenner: @ef4 I think your best bet is to unit test the function is question, that should be simpler then the larger acceptance test and the `PackageCache` stuff.
ef4: Yes, I am curious whether this will be happy in windows, what with a symlink checked into the repo.
stefanpenner: > Yes, I am curious whether this will be happy in windows, what with a symlink checked into the repo.

It looks un-happy.
kellyselden: Would it be possible to create the symlink during the test, maybe in project tmp/?
rwjblue: This is getting stale, but still seems to be some thing that we need.  @ef4 - Any chance you can bring this back up to date with master and kick off another test?

Patch:
diff --git a/lib/models/addon-discovery.js b/lib/models/addon-discovery.js
index 2b3459f06..7e3d59a24 100644
--- a/lib/models/addon-discovery.js
+++ b/lib/models/addon-discovery.js
@@ -9,6 +9,7 @@ const existsSync = require('exists-sync');
 const path = require('path');
 const resolve = require('resolve');
 const heimdall = require('heimdalljs');
+const fs = require('fs');
 
 /**
   AddonDiscovery is responsible for collecting information about all of the
@@ -198,7 +199,7 @@ class AddonDiscovery {
         logger.info(' - is addon, adding...');
         let addonInfo = {
           name: addonPkg.name,
-          path: addonPath,
+          path: fs.realpathSync(addonPath),
           pkg: addonPkg,
         };
         return addonInfo;


Test patch:
diff --git a/tests/fixtures/addon/simple/node_modules/.gitignore b/tests/fixtures/addon/simple/node_modules/.gitignore
new file mode 100644
index 000000000..af25caa4e
--- /dev/null
+++ b/tests/fixtures/addon/simple/node_modules/.gitignore
@@ -0,0 +1,1 @@
+symlinked-addon
\ No newline at end of file
diff --git a/tests/unit/models/addon-discovery-test.js b/tests/unit/models/addon-discovery-test.js
index a354b2924..e0a537eef 100644
--- a/tests/unit/models/addon-discovery-test.js
+++ b/tests/unit/models/addon-discovery-test.js
@@ -7,6 +7,7 @@ const AddonDiscovery = require('../../../lib/models/addon-discovery');
 let fixturePath = path.resolve(__dirname, '../../fixtures/addon');
 const MockUI = require('console-ui/mock');
 const MockCLI = require('../../helpers/mock-cli');
+const fs = require('fs');
 
 describe('models/addon-discovery.js', function() {
   let project, projectPath, ui;
@@ -461,6 +462,32 @@ describe('models/addon-discovery.js', function() {
       expect(result.pkg).to.deep.equal(addonPkg);
     });
 
+    it('discovered path resolves symlinks', function() {
+      let addonPath = path.join(fixturePath, 'simple/node_modules/symlinked-addon');
+      let targetPath = path.join(fixturePath, 'simple/node_modules/ember-random-addon');
+
+      try {
+        // remove any prior symlink that's floating around here
+        fs.unlinkSync(addonPath);
+      } catch (err) {
+        // allowed to fail because symlink may not have existed
+      }
+
+      // Make our symlink. We do it here instead of committing the
+      // symlink to our repo because checking out repos with symlinks
+      // does the wrong thing in Windows non-administrator shells.
+      fs.symlinkSync(targetPath, addonPath);
+
+      const addonPkg = require(path.join(addonPath, 'package.json'));
+      let discovery = new AddonDiscovery(ui);
+
+      let result = discovery.discoverAtPath(addonPath);
+
+      expect(result.name).to.equal('ember-random-addon');
+      expect(result.path).to.equal(addonPath.replace('symlinked-addon', 'ember-random-addon'));
+      expect(result.pkg).to.deep.equal(addonPkg);
+    });
+
     it('returns `null` if path is not for an addon', function() {
       let addonPath = path.join(fixturePath, 'simple');
       let discovery = new AddonDiscovery(ui);