Error on name collisions

Summary: @​public
This moves us from warnings on name collisions to errors. If the error happens in initialization it will fatal out.

However, if the error happens while working (after initialization) then I did my best to make it recoverable. The rational behind this is that if you're working and you're changing names, you may introduce a duplication while moving things around. It will suck if you have to restart the server every time you do that.

Reviewed By: @frantic

Differential Revision: D2493098

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

@@ -13,6 +13,7 @@ const AssetModule_DEPRECATED = require('../AssetModule_DEPRECATED');
 const Fastfs = require('../fastfs');
 const debug = require('debug')('ReactNativePackager:DependencyGraph');
 const path = require('path');
+const Promise = require('promise');
 
 class DeprecatedAssetMap {
   constructor({ fsCrawl, roots, assetExts, fileWatcher, ignoreFilePath, helpers }) {

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

@@ -8,9 +8,9 @@
  */
 'use strict';
 
-const chalk = require('chalk');
 const path = require('path');
 const getPlatformExtension = require('../../lib/getPlatformExtension');
+const Promise = require('promise');
 
 const GENERIC_PLATFORM = 'generic';
 
@@ -19,11 +19,11 @@ class HasteMap {
     this._fastfs = fastfs;
     this._moduleCache = moduleCache;
     this._helpers = helpers;
-    this._map = Object.create(null);
-    this._warnedAbout = Object.create(null);
   }
 
   build() {
+    this._map = Object.create(null);
+
     let promises = this._fastfs.findFilesByExt('js', {
       ignore: (file) => this._helpers.isNodeModulesDir(file)
     }).map(file => this._processHasteModule(file));
@@ -39,20 +39,15 @@ class HasteMap {
 
   processFileChange(type, absPath) {
     return Promise.resolve().then(() => {
-      // Rewarn after file changes.
-      this._warnedAbout = Object.create(null);
-
       /*eslint no-labels: 0 */
       if (type === 'delete' || type === 'change') {
         loop: for (let name in this._map) {
           const modulesMap = this._map[name];
           for (let platform in modulesMap) {
-            const modules = modulesMap[platform];
+            const module = modulesMap[platform];
-            for (var i = 0; i < modules.length; i++) {
+            if (module.path === absPath) {
-              if (modules[i].path === absPath) {
+              delete modulesMap[platform];
-                modules.splice(i, 1);
+              break loop;
-                break loop;
-              }
             }
           }
         }
@@ -82,38 +77,15 @@ class HasteMap {
     // If no platform is given we choose the generic platform module list.
     // If a platform is given and no modules exist we fallback
     // to the generic platform module list.
-    let modules;
     if (platform == null) {
-      modules = modulesMap[GENERIC_PLATFORM];
+      return modulesMap[GENERIC_PLATFORM];
     } else {
-      modules = modulesMap[platform];
+      let module = modulesMap[platform];
-      if (modules == null) {
+      if (module == null) {
-        modules = modulesMap[GENERIC_PLATFORM];
+        module = modulesMap[GENERIC_PLATFORM];
       }
+      return module;
     }
-
-    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'
-          ),
-          name,
-          modules.map(m => m.path).join('\n'),
-        );
-      }
-
-      return modules[0];
-    }
-
-    return modules[0];
   }
 
   _processHasteModule(file) {
@@ -147,11 +119,14 @@ class HasteMap {
     const moduleMap = this._map[name];
     const modulePlatform = getPlatformExtension(mod.path) || GENERIC_PLATFORM;
 
-    if (!moduleMap[modulePlatform]) {
+    if (moduleMap[modulePlatform]) {
-      moduleMap[modulePlatform] = [];
+      throw new Error(
+        `Naming collision detected: ${mod.path} ` +
+        `collides with ${moduleMap[modulePlatform].path}`
+      );
     }
 
-    moduleMap[modulePlatform].push(mod);
+    moduleMap[modulePlatform] = mod;
   }
 }
 

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

@@ -13,6 +13,7 @@ const util = require('util');
 const path = require('path');
 const isAbsolutePath = require('absolute-path');
 const getAssetDataFromName = require('../../lib/getAssetDataFromName');
+const Promise = require('promise');
 
 class ResolutionRequest {
   constructor({

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

@@ -1070,7 +1070,7 @@ describe('DependencyGraph', function() {
       });
     });
 
-    pit('can have multiple modules with the same name', function() {
+    pit('should fatal on multiple modules with the same name', function() {
       var root = '/root';
       fs.__setMockFilesystem({
         'root': {
@@ -1078,59 +1078,33 @@ describe('DependencyGraph', function() {
             '/**',
             ' * @providesModule index',
             ' */',
-            'require("b")',
           ].join('\n'),
           'b.js': [
             '/**',
-            ' * @providesModule b',
+            ' * @providesModule index',
             ' */',
           ].join('\n'),
-          'c.js': [
-            '/**',
-            ' * @providesModule c',
-            ' */',
-          ].join('\n'),
-          'somedir': {
-            'somefile.js': [
-              '/**',
-              ' * @providesModule index',
-              ' */',
-              'require("c")',
-            ].join('\n')
-          }
         }
       });
 
+      const _exit = process.exit;
+      const _error = console.error;
+
+      process.exit = jest.genMockFn();
+      console.error = jest.genMockFn();
+
       var dgraph = new DependencyGraph({
         roots: [root],
         fileWatcher: fileWatcher,
         assetExts: ['png', 'jpg'],
         cache: cache,
       });
-      return getOrderedDependenciesAsJSON(dgraph, '/root/somedir/somefile.js').then(function(deps) {
+
-        expect(deps)
+      return dgraph.load().catch(() => {
-          .toEqual([
+        expect(process.exit).toBeCalledWith(1);
-            {
+        expect(console.error).toBeCalled();
-              id: 'index',
+        process.exit = _exit;
-              path: '/root/somedir/somefile.js',
+        console.error = _error;
-              dependencies: ['c'],
-              isAsset: false,
-              isAsset_DEPRECATED: false,
-              isJSON: false,
-              isPolyfill: false,
-              resolution: undefined,
-            },
-            {
-              id: 'c',
-              path: '/root/c.js',
-              dependencies: [],
-              isAsset: false,
-              isAsset_DEPRECATED: false,
-              isJSON: false,
-              isPolyfill: false,
-              resolution: undefined,
-            },
-          ]);
       });
     });
 
@@ -3629,88 +3603,6 @@ 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';

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

@@ -73,7 +73,11 @@ class DependencyGraph {
     this._opts = validateOpts(options);
     this._cache = this._opts.cache;
     this._helpers = new Helpers(this._opts);
-    this.load();
+    this.load().catch((err) => {
+      // This only happens at initialization. Live errors are easier to recover from.
+      console.error('Error building DepdendencyGraph:\n', err.stack);
+      process.exit(1);
+    });
   }
 
   load() {
@@ -198,9 +202,25 @@ class DependencyGraph {
       return;
     }
 
-    this._loading = this._loading.then(
+    // Ok, this is some tricky promise code. Our requirements are:
-      () => this._hasteMap.processFileChange(type, absPath)
+    // * we need to report back failures
-    );
+    // * failures shouldn't block recovery
+    // * Errors can leave `hasteMap` in an incorrect state, and we need to rebuild
+    // After we process a file change we record any errors which will also be
+    // reported via the next request. On the next file change, we'll see that
+    // we are in an error state and we should decide to do a full rebuild.
+    this._loading = this._loading.finally(() => {
+      if (this._hasteMapError) {
+        this._hasteMapError = null;
+        // Rebuild the entire map if last change resulted in an error.
+        console.warn('Rebuilding haste map to recover from error');
+        this._loading = this._hasteMap.build();
+      } else {
+        this._loading = this._hasteMap.processFileChange(type, absPath);
+        this._loading.catch((e) => this._hasteMapError = e);
+      }
+      return this._loading;
+    });
   }
 }