Fix haste resolution (and better warnings)

Summary: @​public

Fix the haste resolution algorithm. Changed the map data structure from a list of modules, to a list of modules grouped by platform.
This considerably simplifies the "getModule" method and makes it easy to properly warn about colliding module names.
This also fixes a bug where we used to include `.web` files when we shouldn't (see task).

Reviewed By: @vjeux

Differential Revision: D2482969

packager/react-packager/src/DependencyResolver/DependencyGraph/HasteMap.js

@@ -12,6 +12,8 @@ const chalk = require('chalk');
 const path = require('path');
 const getPlatformExtension = require('../../lib/getPlatformExtension');
 
+const GENERIC_PLATFORM = 'generic';
+
 class HasteMap {
   constructor({ fastfs, moduleCache, helpers }) {
     this._fastfs = fastfs;
@@ -43,11 +45,14 @@ class HasteMap {
       /*eslint no-labels: 0 */
       if (type === 'delete' || type === 'change') {
         loop: for (let name in this._map) {
-          let modules = this._map[name];
+          const modulesMap = this._map[name];
-          for (var i = 0; i < modules.length; i++) {
+          for (let platform in modulesMap) {
-            if (modules[i].path === absPath) {
+            const modules = modulesMap[platform];
-              modules.splice(i, 1);
+            for (var i = 0; i < modules.length; i++) {
-              break loop;
+              if (modules[i].path === absPath) {
+                modules.splice(i, 1);
+                break loop;
+              }
             }
           }
         }
@@ -69,36 +74,45 @@ class HasteMap {
   }
 
   getModule(name, platform = null) {
-    if (!this._map[name]) {
+    const modulesMap = this._map[name];
+    if (modulesMap == null) {
       return null;
     }
 
-    const modules = this._map[name];
+    // If no platform is given we choose the generic platform module list.
-    if (platform != null) {
+    // If a platform is given and no modules exist we fallback
-      for (let i = 0; i < modules.length; i++) {
+    // to the generic platform module list.
-        if (getPlatformExtension(modules[i].path) === platform) {
+    let modules;
-          return modules[i];
+    if (platform == null) {
-        }
+      modules = modulesMap[GENERIC_PLATFORM];
+    } else {
+      modules = modulesMap[platform];
+      if (modules == null) {
+        modules = modulesMap[GENERIC_PLATFORM];
+      }
+    }
+
+    if (modules == null) {
+      return null;
+    }
+
+    if (modules.length > 1) {
+      if (!this._warnedAbout[name]) {
+        this._warnedAbout[name] = true;
+        console.warn(
+          chalk.yellow(
+            '\nWARNING: Found multiple haste modules or packages ' +
+            'with the name `%s`. Please fix this by adding it to ' +
+            'the blacklist or deleting the modules keeping only one.\n' +
+            'One of the following modules will be selected at random:\n%s\n'
+          ),
+          name,
+          modules.map(m => m.path).join('\n'),
+        );
       }
 
-      if (modules.length > 1) {
+      const randomIndex = Math.floor(Math.random() * modules.length);
-        if (!this._warnedAbout[name]) {
+      return modules[randomIndex];
-          this._warnedAbout[name] = true;
-          console.warn(
-            chalk.yellow(
-              '\nWARNING: Found multiple haste modules or packages ' +
-              'with the name `%s`. Please fix this by adding it to ' +
-              'the blacklist or deleting the modules keeping only one.\n' +
-              'One of the following modules will be selected at random:\n%s\n'
-            ),
-            name,
-            modules.map(m => m.path).join('\n'),
-          );
-        }
-
-        const randomIndex = Math.floor(Math.random() * modules.length);
-        return modules[randomIndex];
-      }
     }
 
     return modules[0];
@@ -129,15 +143,17 @@ class HasteMap {
 
   _updateHasteMap(name, mod) {
     if (this._map[name] == null) {
-      this._map[name] = [];
+      this._map[name] = Object.create(null);
     }
 
-    if (mod.type === 'Module') {
+    const moduleMap = this._map[name];
-      // Modules takes precendence over packages.
+    const modulePlatform = getPlatformExtension(mod.path) || GENERIC_PLATFORM;
-      this._map[name].unshift(mod);
+
-    } else {
+    if (!moduleMap[modulePlatform]) {
-      this._map[name].push(mod);
+      moduleMap[modulePlatform] = [];
     }
+
+    moduleMap[modulePlatform].push(mod);
   }
 }
 

packager/react-packager/src/DependencyResolver/DependencyGraph/__tests__/DependencyGraph-test.js

@@ -1134,66 +1134,6 @@ describe('DependencyGraph', function() {
       });
     });
 
-    pit('providesModule wins when conflict with package', function() {
-      var root = '/root';
-      fs.__setMockFilesystem({
-        'root': {
-          'index.js': [
-            '/**',
-            ' * @providesModule index',
-            ' */',
-            'require("aPackage")',
-          ].join('\n'),
-          'b.js': [
-            '/**',
-            ' * @providesModule aPackage',
-            ' */',
-          ].join('\n'),
-          'aPackage': {
-            'package.json': JSON.stringify({
-              name: 'aPackage',
-              main: 'main.js'
-            }),
-            'main.js': 'lol'
-          }
-        }
-      });
-
-      var dgraph = new DependencyGraph({
-        roots: [root],
-        fileWatcher: fileWatcher,
-        assetExts: ['png', 'jpg'],
-        cache: cache,
-      });
-      return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) {
-        expect(deps)
-          .toEqual([
-            {
-              id: 'index',
-              path: '/root/index.js',
-              dependencies: ['aPackage'],
-              isAsset: false,
-              isAsset_DEPRECATED: false,
-              isJSON: false,
-              isPolyfill: false,
-              resolution: undefined,
-              resolveDependency: undefined,
-            },
-            {
-              id: 'aPackage',
-              path: '/root/b.js',
-              dependencies: [],
-              isAsset: false,
-              isAsset_DEPRECATED: false,
-              isJSON: false,
-              isPolyfill: false,
-              resolution: undefined,
-              resolveDependency: undefined,
-            },
-          ]);
-      });
-    });
-
     pit('should be forgiving with missing requires', function() {
       var root = '/root';
       fs.__setMockFilesystem({
@@ -2616,6 +2556,11 @@ describe('DependencyGraph', function() {
              * @providesModule a
              */
           `,
+          'a.web.js': `
+            /**
+             * @providesModule a
+             */
+          `,
         }
       });
 
@@ -3684,6 +3629,88 @@ describe('DependencyGraph', function() {
     });
   });
 
+  describe('warnings', () => {
+    let warn = console.warn;
+
+    beforeEach(() => {
+      console.warn = jest.genMockFn();
+    });
+
+    afterEach(() => {
+      console.warn = warn;
+    });
+
+    pit('should warn about colliding module names', function() {
+      fs.__setMockFilesystem({
+        'root': {
+          'index.js': `
+            /**
+             * @providesModule index
+             */
+             require('a');
+          `,
+          'a.js': `
+            /**
+             * @providesModule a
+             */
+          `,
+          'b.js': `
+            /**
+             * @providesModule a
+             */
+          `,
+        }
+      });
+
+      var dgraph = new DependencyGraph({
+        roots: ['/root'],
+        fileWatcher: fileWatcher,
+        assetExts: ['png', 'jpg'],
+        cache: cache,
+      });
+
+      return getOrderedDependenciesAsJSON(dgraph, '/root/index.js').then(function(deps) {
+        expect(console.warn.mock.calls.length).toBe(1);
+      });
+    });
+
+
+    pit('should warn about colliding module names within a platform', function() {
+      var root = '/root';
+      fs.__setMockFilesystem({
+        'root': {
+          'index.ios.js': `
+            /**
+             * @providesModule index
+             */
+             require('a');
+          `,
+          'a.ios.js': `
+            /**
+             * @providesModule a
+             */
+          `,
+          'b.ios.js': `
+            /**
+             * @providesModule a
+             */
+          `,
+        }
+      });
+
+      var dgraph = new DependencyGraph({
+        roots: [root],
+        fileWatcher: fileWatcher,
+        assetExts: ['png', 'jpg'],
+        cache: cache,
+      });
+
+      return getOrderedDependenciesAsJSON(dgraph, '/root/index.ios.js', 'ios').then(function(deps) {
+        expect(console.warn.mock.calls.length).toBe(1);
+      });
+    });
+  });
+
   describe('getAsyncDependencies', () => {
     pit('should get dependencies', function() {
       var root = '/root';