Repository: ember-cli/ember-cli
Pull request #8959 (2019-12-07 01:20:29)

Non-scope directories without a package.json should not be considered a node_module
…ge.json should not be considered a node_module

Sometimes dependency management tooling may leave empty directories in node_modules. This is likely a bug in their implementation, but as it turns out the node resolution algorithm accounts for it, by simply not considering node_modules who do not have a package.json.

Assuming this works as expected, and folks are onboard with the change, this is likely a candidate for a `[Bugfix release]` or similar backport.

This, in theory and pending additional local testing, should address https://github.com/ember-engines/ember-engines/issues/689

- [x] review
- [x] verify fix, now that it is code-complete and not a quick hack
- [x] decide how far back to backport
- [x] fix-up windows tests.

rwjblue: > this is likely a candidate for a [Bugfix release] or similar backport.

Yeah, I actually think we should consider making an LTS back port (since I’m pretty sure 3.12 is also affected)...
stefanpenner: I'm busy tomorrow, will complete once i have time again.
stefanpenner: Confirmed This fix addresses the issue I saw in ember-engines, currently fixing up 2 small windows failures.

I'll likely also open a yarn issue, as it should likely not be leaving empty directories floating around...
rwjblue: > fix-up windows tests.

I'm not sure that they are actually broken. They take too long, and timeout when the AppVeyor boxes are under load, and pass when they are not busy (e.g. mid-night).

From the appveyor log:
> Build execution time has reached the maximum allowed time for your plan (60 minutes).

stefanpenner: I'm passing this off to @rwjblue, as he has a specific plan for releasing/backports.

Patch:
diff --git a/lib/models/package-info-cache/index.js b/lib/models/package-info-cache/index.js
index e032345bb..3b8b340be 100644
--- a/lib/models/package-info-cache/index.js
+++ b/lib/models/package-info-cache/index.js
@@ -636,18 +636,25 @@ class PackageInfoCache {
     logger.info('Creating new NodeModulesList instance for %o', realPath);
     nodeModulesList = new NodeModulesList(realPath, this);
 
-    let entries = fs.readdirSync(realPath); // should not fail because getRealDirectoryPath passed
+    const entries = fs.readdirSync(realPath).filter(fileName => {
+      if (fileName.startsWith('.') || fileName.startsWith('_')) {
+        // we explicitly want to ignore these, according to the
+        // definition of a valid package name.
+        return false;
+      } else if (fileName.startsWith('@')) {
+        return true;
+      } else if (!fs.existsSync(`${realPath}/${fileName}/package.json`)) {
+        // a node_module is only valid if it contains a package.json
+        return false;
+      } else {
+        return true;
+      }
+    }); // should not fail because getRealDirectoryPath passed
 
     entries.forEach(entryName => {
       // entries should be either a package or a scoping directory. I think
       // there can also be files, but we'll ignore those.
 
-      if (entryName.startsWith('.') || entryName.startsWith('_')) {
-        // we explicitly want to ignore these, according to the
-        // definition of a valid package name.
-        return;
-      }
-
       let entryPath = path.join(realPath, entryName);
 
       if (getRealFilePath(entryPath)) {


Test patch:
diff --git a/tests/unit/models/package-info-cache/package-info-cache-test.js b/tests/unit/models/package-info-cache/package-info-cache-test.js
index 5648d608b..cbe5fb903 100644
--- a/tests/unit/models/package-info-cache/package-info-cache-test.js
+++ b/tests/unit/models/package-info-cache/package-info-cache-test.js
@@ -181,7 +181,18 @@ describe('models/package-info-cache/package-info-cache-test.js', function() {
         // create a new ember-app
         fixturifyProject = new FixturifyProject('simple-ember-app', '0.0.0', project => {
           project.addAddon('ember-resolver', '^5.0.1');
-          project.addAddon('ember-random-addon', 'latest');
+          project.addAddon('ember-random-addon', 'latest', addon => {
+            addon.addAddon('other-nested-addon', 'latest', addon => {
+              addon.addAddon('ember-resolver', '*');
+              addon.toJSON = function() {
+                const json = Object.getPrototypeOf(this).toJSON.call(this);
+                // here we introduce an empty folder in our node_modules.
+                json[this.name].node_modules['ember-resolver'] = {};
+                return json;
+              };
+            });
+          });
+
           project.addAddon('loader.js', 'latest');
           project.addAddon('something-else', 'latest');
 
@@ -202,6 +213,14 @@ describe('models/package-info-cache/package-info-cache-test.js', function() {
         fixturifyProject.dispose();
       });
 
+      it('was able to find ember-resolver even if an empty directory was left', function() {
+        const emberResolver = project.findAddonByName('ember-resolver');
+        const nestedEmberResolver = project.findAddonByName('ember-random-addon').addons[0].addons[0];
+        expect(emberResolver.name).to.eql('ember-resolver');
+        expect(nestedEmberResolver.name).to.eql('ember-resolver');
+        expect(emberResolver.root).to.eql(nestedEmberResolver.root);
+      });
+
       it('has dependencies who have their mayHaveAddons correctly set', function() {
         expect(projectPackageInfo.devDependencyPackages['non-ember-thingy']).to.have.property('mayHaveAddons', false);
         expect(projectPackageInfo.devDependencyPackages['ember-cli']).to.have.property('mayHaveAddons', false);