Skip to content

Commit

Permalink
Fix inconsistent filesystem state after a Haste collision (#1399)
Browse files Browse the repository at this point in the history 
Summary:
Prior to D45033364 / f443e5d, Haste collisions arising after startup (in watch mode) would log a warning to console, but would otherwise be fully processed. This was true even if `throwOnModuleCollisions` was true, because `_watch()` **mutates `this._options.throwOnModuleCollision`**:

https://github.com/facebook/metro/blob/462cc9dbaca3dad0320ea7cdc9fd56593992268c/packages/metro-file-map/src/index.js#L872-L874

In D45033364, this behaviour unintentionally regressed, because `MutableHasteMap`, newly responsible for warning/throwing, is passed `throwOnModuleCollision` (by its original value) in its constructor, and so the mutation in `_watch` had no effect:

https://github.com/facebook/metro/blob/462cc9dbaca3dad0320ea7cdc9fd56593992268c/packages/metro-file-map/src/index.js#L415-L420

Weirdly, I apparently did realise this at the time, because I gave `MutableHasteMap` a `setThrowsOnModuleCollision` method that has never been used, maybe the call got lost in a merge or something 🤷‍♂️:

https://github.com/facebook/metro/blob/462cc9dbaca3dad0320ea7cdc9fd56593992268c/packages/metro-file-map/src/lib/MutableHasteMap.js#L287-L289

The throw during watch does not actually kill Metro, it just gets logged:

https://github.com/facebook/metro/blob/462cc9dbaca3dad0320ea7cdc9fd56593992268c/packages/metro-file-map/src/index.js#L1071-L1075

**But** critically, a throw like this during `_processFile` prevents items being added to the `fileSystem` (`TreeFS`) and prevents change events firing:

https://github.com/facebook/metro/blob/462cc9dbaca3dad0320ea7cdc9fd56593992268c/packages/metro-file-map/src/index.js#L1032-L1042

And worse, because the file is not in `TreeFS`, deleting it will *not* remove it from the Haste map (due to the early return in `_removeIfExists`), so the collision cannot be resolved without restarting Metro:

https://github.com/facebook/metro/blob/462cc9dbaca3dad0320ea7cdc9fd56593992268c/packages/metro-file-map/src/index.js#L826-L841

This is frequently seen when rebasing/moving around revisions while Metro is running, and could cause hard-to-debug errors (missing or ambiguous Haste resolution errors) that mysteriously resolve themselves by restarting Metro.

This restores the old behaviour in the simplest way by calling the setter at the same time as mutating `_options`, as well as adding a regression test. We'll refactor this a bit later.

Changelog:
```
 - **[Fix]**: Haste conflicts created after startup could cause inconsistent filesystem state
```

Pull Request resolved: #1399

Test Plan:
#### Before (with new test)
```
FAIL packages/metro-file-map/src/__tests__/index-test.js
  FileMap
  <...>
      ✓ correctly handles moving a Haste module (39 ms)
      recovery from duplicate module IDs
        ✕ does not throw on a duplicate created at runtime even if throwOnModuleCollision: true (9 ms)
        ✓ recovers when the oldest version of the duplicates is fixed (68 ms)
        ✓ recovers when the most recent duplicate is fixed (69 ms)
        ✓ ignore directory events (even with file-ish names) (38 ms)

  ● FileMap › file system changes processing › recovery from duplicate module IDs › does not throw on a duplicate created at runtime even if throwOnModuleCollision: true

    should not print error

      2087 |           await new Promise((resolve, reject) => {
      2088 |             console.error.mockImplementationOnce(() => {
    > 2089 |               reject(new Error('should not print error'));
           |                      ^
      2090 |             });
      2091 |             hm.once('change', resolve);
      2092 |           });

      at console.<anonymous> (packages/metro-file-map/src/__tests__/index-test.js:2089:22)
      at MutableHasteMap.setModule (packages/metro-file-map/src/lib/MutableHasteMap.js:220:30)
      at setModule (packages/metro-file-map/src/index.js:551:18)
      at packages/metro-file-map/src/index.js:1034:15

Test Suites: 1 failed, 1 total
Tests:       1 failed, 47 passed, 48 total
Snapshots:   5 passed, 5 total
Time:        2.232 s, estimated 3 s
Ran all test suites matching /metro-file-map\/src\/__tests__\/index-test/i.
error Command failed with exit code 1.
```

 - Simulate copying a Haste module and then deleting it from the original location
 - Observe Metro does not have the new Haste module.
```
fbsource  touch xplat/js/RKJSModules/Apps/Playground.js
 fbsource  rm xplat/js/RKJSModules/Apps/Playground/Components/Playground.js
 fbsource  curl http://localhost:8081/RKJSModules/Apps/Playground/Components/PlaygroundSurface.bundle\?platform\=android
{"originModulePath":"/Users/robhogan/fbsource/xplat/js/RKJSModules/Apps/Playground/Components/PlaygroundSurface.js","targetModuleName":"Playground","message":"Unable to resolve module Playground from /Users/robhogan/fbsource/xplat/js/RKJSModules/Apps/Playground/Components/PlaygroundSurface.js: Playground could not be found within the project or in these directories...
```

```
  Metro:WatchmanWatcher Handling change to: RKJSModules/Apps/Playground.js (new: true, exists: true, type: f) +0ms
metro-file-map: Haste module naming collision: xplat:Playground
  The following files share their name; please adjust your hasteImpl:
    * <rootDir>/RKJSModules/Apps/Playground/Components/Playground.js
    * <rootDir>/RKJSModules/Apps/Playground.js

metro-file-map: watch error:
  Error: Duplicated files or mocks. Please check the console for more info
    at MutableHasteMap.setModule (/Users/robhogan/Library/Caches/dotslash/obj/cas/46/3b50844ce2feecf877e23a1d785b4d03/extract/tar.gz/metro-file-map/src/lib/MutableHasteMap.js:232:15)
    at workerReply (/Users/robhogan/Library/Caches/dotslash/obj/cas/46/3b50844ce2feecf877e23a1d785b4d03/extract/tar.gz/metro-file-map/src/index.js:551:18)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async /Users/robhogan/Library/Caches/dotslash/obj/cas/46/3b50844ce2feecf877e23a1d785b4d03/extract/tar.gz/metro-file-map/src/index.js:1033:13

...

  Metro:WatchmanWatcher Handling change to: RKJSModules/Apps/Playground/Components/Playground.js (new: false, exists: false, type: f) +0ms
  Metro:DeltaCalculator Handling delete: /Users/robhogan/fbsource/xplat/js/RKJSModules/Apps/Playground/Components/Playground.js (type: f) +3ms

...

 BUNDLE  RKJSModules/Apps/Playground/Components/PlaygroundSurface.js

 ERROR  Error: Unable to resolve module Playground from /Users/robhogan/fbsource/xplat/js/RKJSModules/Apps/Playground/Components/PlaygroundSurface.js: Playground could not be found within the project or in these directories:
  public/node_modules
  10 | import FabricNavigationOptions from 'FabricNavigationOptions';
  11 | import {useNavigation} from 'FBNavigation';
> 12 | import Playground from 'Playground';
```

#### After

 - Tests pass
 - Collision is processed as a warning (no "watch error" printed)
```
Metro:WatchmanWatcher Received subscription response: metro-file-map-33628--Users-robhogan-fbsource-xplat-js-698a63ea9501dbe283dee0e716580ebe (fresh: false, files: 1, enter: undefined, leave: undefined) +18s
  Metro:WatchmanWatcher Handling change to: RKJSModules/Apps/Playground.js (new: true, exists: true, type: f) +0ms
metro-file-map: Haste module naming collision: xplat:Playground
  The following files share their name; please adjust your hasteImpl:
    * <rootDir>/RKJSModules/Apps/Playground/Components/Playground.js
    * <rootDir>/RKJSModules/Apps/Playground.js
```
 - Bundling succeeds after fixing collision:
```
 fbsource  touch xplat/js/RKJSModules/Apps/Playground.js
 fbsource  rm xplat/js/RKJSModules/Apps/Playground/Components/Playground.js
 fbsource  curl -I http://localhost:8081/RKJSModules/Apps/Playground/Components/PlaygroundSurface.bundle\?platform\=android
HTTP/1.1 200 OK
```

Reviewed By: vzaidman

Differential Revision: D67027485

Pulled By: robhogan

fbshipit-source-id: d8250f4a88dee18646e33e73a64d434155551667
  • Loading branch information
@robhogan @facebook-github-bot
robhogan authored and facebook-github-bot committed Dec 10, 2024
1 parent c54175a commit 44bab52
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 0 deletions.
55 changes: 55 additions & 0 deletions packages/metro-file-map/src/__tests__/index-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2059,6 +2059,61 @@ describe('FileMap', () => {
}
}

fm_it(
'does not throw on a duplicate created at runtime even if throwOnModuleCollision: true',
async hm => {
mockFs[path.join('/', 'project', 'fruits', 'Pear.js')] = `
// Pear!
`;
mockFs[path.join('/', 'project', 'fruits', 'another', 'Pear.js')] = `
// Pear too!
`;
const {fileSystem} = await hm.build();
const e = mockEmitters[path.join('/', 'project', 'fruits')];
e.emit(
'all',
'change',
'Pear.js',
path.join('/', 'project', 'fruits'),
MOCK_CHANGE_FILE,
);
e.emit(
'all',
'add',
'Pear.js',
path.join('/', 'project', 'fruits', 'another'),
MOCK_CHANGE_FILE,
);
await new Promise((resolve, reject) => {
console.error.mockImplementationOnce(() => {
reject(new Error('should not print error'));
});
hm.once('change', resolve);
});
// Expect a warning to be printed, but no error.
expect(console.warn).toHaveBeenCalledWith(
expect.stringContaining(
'metro-file-map: Haste module naming collision: Pear',
),
);
// Both files should be added to the fileSystem, despite the Haste
// collision
expect(
fileSystem.exists(path.join('/', 'project', 'fruits', 'Pear.js')),
).toBe(true);
expect(
fileSystem.exists(
path.join('/', 'project', 'fruits', 'another', 'Pear.js'),
),
).toBe(true);
},
{
config: {
throwOnModuleCollision: true,
},
},
);

fm_it(
'recovers when the oldest version of the duplicates is fixed',
async hm => {
Expand Down
1 change: 1 addition & 0 deletions packages/metro-file-map/src/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,7 @@ export default class FileMap extends EventEmitter {
// In watch mode, we'll only warn about module collisions and we'll retain
// all files, even changes to node_modules.
this._options.throwOnModuleCollision = false;
hasteMap.setThrowOnModuleCollision(false);
this._options.retainAllFiles = true;

const hasWatchedExtension = (filePath: string) =>
Expand Down

0 comments on commit 44bab52

Please sign in to comment.