From 44bab52cf10a70063e6d68d1e0750fa8d399f3ed Mon Sep 17 00:00:00 2001 From: Rob Hogan <2590098+robhogan@users.noreply.github.com> Date: Tue, 10 Dec 2024 12:27:46 -0800 Subject: [PATCH] Fix inconsistent filesystem state after a Haste collision (#1399) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Prior to D45033364 / f443e5ddcdaf31748149212c805cebb181a5a365, 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: https://github.com/facebook/metro/pull/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. (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: * /RKJSModules/Apps/Playground/Components/Playground.js * /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: * /RKJSModules/Apps/Playground/Components/Playground.js * /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 --- .../src/__tests__/index-test.js | 55 +++++++++++++++++++ packages/metro-file-map/src/index.js | 1 + 2 files changed, 56 insertions(+) diff --git a/packages/metro-file-map/src/__tests__/index-test.js b/packages/metro-file-map/src/__tests__/index-test.js index f177e5238b..549cf6d7b0 100644 --- a/packages/metro-file-map/src/__tests__/index-test.js +++ b/packages/metro-file-map/src/__tests__/index-test.js @@ -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 => { diff --git a/packages/metro-file-map/src/index.js b/packages/metro-file-map/src/index.js index 3736d69b51..722ffee00b 100644 --- a/packages/metro-file-map/src/index.js +++ b/packages/metro-file-map/src/index.js @@ -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) =>