jest-haste-map: recover from duplicate IDs (#3647)

* jest-haste-map: do not expose any module when ID is duplicate

* jest-haste-map: build up an index of duplicate IDs

* jest-haste-map: recover from duplicate module IDs

* jest-haste-map: fix prettier

* fix all tests+flow

* jest-haste-map: address comments

packages/jest-haste-map/src/__tests__/index-test.js

@@ -353,9 +353,9 @@ describe('HasteMap', () => {
     return new HasteMap(defaultConfig)
       .build()
       .then(({__hasteMapForTest: data}) => {
-        expect(data.map.Strawberry[H.GENERIC_PLATFORM][0]).toEqual(
-          '/fruits/raspberry.js',
-        );
+        // Duplicate modules are removed so that it doesn't cause
+        // non-determinism later on.
+        expect(data.map.Strawberry[H.GENERIC_PLATFORM]).not.toBeDefined();
 
         expect(console.warn.mock.calls[0][0]).toMatchSnapshot();
       });
@@ -761,7 +761,7 @@ describe('HasteMap', () => {
       },
     );
 
-    describe('recovery from duplicate module IDs (broken right now)', () => {
+    describe('recovery from duplicate module IDs', () => {
       async function setupDuplicates(hm) {
         mockFs['/fruits/pear.js'] = [
           '/**',
@@ -778,8 +778,7 @@ describe('HasteMap', () => {
         e.emit('all', 'add', 'blueberry.js', '/fruits', MOCK_STAT);
         const {hasteFS, moduleMap} = await waitForItToChange(hm);
         expect(hasteFS.exists('/fruits/blueberry.js')).toBe(true);
-        // should be `null`
-        expect(moduleMap.getModule('Pear')).not.toBe(null);
+        expect(moduleMap.getModule('Pear')).toBe(null);
       }
 
       hm_it(
@@ -794,8 +793,7 @@ describe('HasteMap', () => {
           const e = mockEmitters['/fruits'];
           e.emit('all', 'change', 'pear.js', '/fruits', MOCK_STAT);
           const {moduleMap} = await waitForItToChange(hm);
-          // should be "/fruits/blueberry.js"
-          expect(moduleMap.getModule('Pear')).toBe(null);
+          expect(moduleMap.getModule('Pear')).toBe('/fruits/blueberry.js');
           expect(moduleMap.getModule('OldPear')).toBe('/fruits/pear.js');
           expect(moduleMap.getModule('Blueberry')).toBe(null);
         },
@@ -811,8 +809,7 @@ describe('HasteMap', () => {
         const e = mockEmitters['/fruits'];
         e.emit('all', 'change', 'blueberry.js', '/fruits', MOCK_STAT);
         const {moduleMap} = await waitForItToChange(hm);
-        // should be "/fruits/pear.js"
-        expect(moduleMap.getModule('Pear')).toBe(null);
+        expect(moduleMap.getModule('Pear')).toBe('/fruits/pear.js');
         expect(moduleMap.getModule('Blueberry')).toBe('/fruits/blueberry.js');
       });
     });

packages/jest-haste-map/src/index.js

@@ -325,6 +325,28 @@ class HasteMap extends EventEmitter {
           throw new Error(message);
         }
         this._console.warn(message);
+        // We do NOT want consumers to use a module that is ambiguous.
+        delete moduleMap[platform];
+        if (Object.keys(moduleMap).length === 1) {
+          delete map[id];
+        }
+        let dupsByPlatform = hasteMap.duplicates[id];
+        if (dupsByPlatform == null) {
+          dupsByPlatform = hasteMap.duplicates[id] = (Object.create(null): any);
+        }
+        const dups = (dupsByPlatform[platform] = (Object.create(null): any));
+        dups[module[H.PATH]] = module[H.TYPE];
+        dups[existingModule[H.PATH]] = existingModule[H.TYPE];
+        return;
+      }
+
+      const dupsByPlatform = hasteMap.duplicates[id];
+      if (dupsByPlatform != null) {
+        const dups = dupsByPlatform[platform];
+        if (dups != null) {
+          dups[module[H.PATH]] = module[H.TYPE];
+        }
+        return;
       }
 
       moduleMap[platform] = module;
@@ -549,8 +571,6 @@ class HasteMap extends EventEmitter {
     // We only need to copy the entire haste map once on every "frame".
     let mustCopy = true;
 
-    const copy = object => Object.assign(Object.create(null), object);
-
     const createWatcher = root => {
       const watcher = new Watcher(root, {
         dot: false,
@@ -619,6 +639,7 @@ class HasteMap extends EventEmitter {
             mustCopy = false;
             hasteMap = {
               clocks: copy(hasteMap.clocks),
+              duplicates: copy(hasteMap.duplicates),
               files: copy(hasteMap.files),
               map: copy(hasteMap.map),
               mocks: copy(hasteMap.mocks),
@@ -640,6 +661,8 @@ class HasteMap extends EventEmitter {
             delete hasteMap.mocks[mockName];
           }
 
+          this._recoverDuplicates(hasteMap, filePath, moduleName);
+
           // If the file was added or changed,
           // parse it and update the haste map.
           if (type === 'add' || type === 'change') {
@@ -669,7 +692,9 @@ class HasteMap extends EventEmitter {
           return null;
         })
         .catch(error => {
-          this._console.error(`jest-haste-map: watch error:\n  ${error}\n`);
+          this._console.error(
+            `jest-haste-map: watch error:\n  ${error.stack}\n`,
+          );
         });
     };
 
@@ -681,6 +706,51 @@ class HasteMap extends EventEmitter {
     });
   }
 
+  /**
+   * This function should be called when the file under `filePath` is removed
+   * or changed. When that happens, we want to figure out if that file was
+   * part of a group of files that had the same ID. If it was, we want to
+   * remove it from the group. Furthermore, if there is only one file
+   * remaining in the group, then we want to restore that single file as the
+   * correct resolution for its ID, and cleanup the duplicates index.
+   */
+  _recoverDuplicates(
+    hasteMap: InternalHasteMap,
+    filePath: string,
+    moduleName: string,
+  ) {
+    let dupsByPlatform = hasteMap.duplicates[moduleName];
+    if (dupsByPlatform == null) {
+      return;
+    }
+    const platform =
+      getPlatformExtension(filePath, this._options.platforms) ||
+      H.GENERIC_PLATFORM;
+    let dups = dupsByPlatform[platform];
+    if (dups == null) {
+      return;
+    }
+    dupsByPlatform = hasteMap.duplicates[moduleName] = (copy(
+      dupsByPlatform,
+    ): any);
+    dups = dupsByPlatform[platform] = (copy(dups): any);
+    const dedupType = dups[filePath];
+    delete dups[filePath];
+    const filePaths = Object.keys(dups);
+    if (filePaths.length > 1) {
+      return;
+    }
+    let dedupMap = hasteMap.map[moduleName];
+    if (dedupMap == null) {
+      dedupMap = hasteMap.map[moduleName] = (Object.create(null): any);
+    }
+    dedupMap[platform] = [filePaths[0], dedupType];
+    delete dupsByPlatform[platform];
+    if (Object.keys(dupsByPlatform).length === 0) {
+      delete hasteMap.duplicates[moduleName];
+    }
+  }
+
   end() {
     clearInterval(this._changeInterval);
     if (!this._watchers.length) {
@@ -725,6 +795,7 @@ class HasteMap extends EventEmitter {
   _createEmptyMap(): InternalHasteMap {
     return {
       clocks: Object.create(null),
+      duplicates: Object.create(null),
       files: Object.create(null),
       map: Object.create(null),
       mocks: Object.create(null),
@@ -735,6 +806,8 @@ class HasteMap extends EventEmitter {
   static ModuleMap: Class<HasteModuleMap>;
 }
 
+const copy = object => Object.assign(Object.create(null), object);
+
 HasteMap.H = H;
 HasteMap.ModuleMap = HasteModuleMap;
 

types/HasteMap.js

@@ -21,8 +21,15 @@ export type ModuleMapData = {[id: string]: ModuleMapItem};
 export type WatchmanClocks = {[filepath: Path]: string};
 export type HasteRegExp = RegExp | ((str: string) => boolean);
 
+export type DuplicatesIndex = {
+  [id: string]: {
+    [platform: string]: {[filePath: string]: /* type */ number},
+  },
+};
+
 export type InternalHasteMap = {|
   clocks: WatchmanClocks,
+  duplicates: DuplicatesIndex,
   files: FileData,
   map: ModuleMapData,
   mocks: MockData,