From 0fc8e45c2f2609fcc52240e554d9a43471c54f84 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Tue, 10 Dec 2024 12:27:46 -0800 Subject: [PATCH] Don't zombie on preexisting Haste collisions on startup Summary: For historical reasons, Metro currently fails "hard" (actually, tries to, but enters a zombie state) when the filesystem contains Haste collisions on startup. This is mostly inherited from Jest, which prefers to fail fast on the Haste (or Jest Mock) map being in a bad state. In Metro, it isn't useful to anyone - the server is designed to gracefully handle temporary Haste collisions - warn about them, fail to resolve ambiguous Haste names with an actionable message, and recover when the collision is fixed. This diff changes the argument to `metro-file-map` to prevent a throw which would effectively hang Metro, allowing it to treat Haste collisions detected on startup exactly the same as a Haste collision created while Metro is running. Changelog: ``` - **[Fix]**: Don't enter zombie state on startup Haste collisions ``` Reviewed By: huntie Differential Revision: D66935485 fbshipit-source-id: 39c664a12bb5be8c12851cab49dec66213daa95a --- .../DeltaBundler/__tests__/resolver-test.js | 124 ++++++++++++++---- .../metro/src/node-haste/DependencyGraph.js | 5 +- 2 files changed, 100 insertions(+), 29 deletions(-) diff --git a/packages/metro/src/DeltaBundler/__tests__/resolver-test.js b/packages/metro/src/DeltaBundler/__tests__/resolver-test.js index a980636d88..970f83f453 100644 --- a/packages/metro/src/DeltaBundler/__tests__/resolver-test.js +++ b/packages/metro/src/DeltaBundler/__tests__/resolver-test.js @@ -16,6 +16,11 @@ import type {TransformResultDependency} from '../types.flow'; import type {InputConfigT} from 'metro-config/src/configTypes.flow'; const {getDefaultConfig, mergeConfig} = require('metro-config'); +const {AmbiguousModuleResolutionError} = require('metro-core'); +const { + DuplicateHasteCandidatesError, + default: {H: Haste}, +} = require('metro-file-map'); const path = require('path'); const mockPlatform = process.platform; @@ -31,7 +36,9 @@ jest endianness: () => 'LE', release: () => '', })) - .mock('graceful-fs', () => require('fs')); + .mock('graceful-fs', () => require('fs')) + .spyOn(console, 'warn') + .mockImplementation(() => {}); jest.setTimeout(10000); @@ -1793,9 +1800,7 @@ function dep(name: string): TransformResultDependency { }); }); - test('fatals on multiple packages with the same name', async () => { - // $FlowFixMe[cannot-write] - console.warn = jest.fn(); + test('resolution throws on multiple packages with the same name', async () => { setMockFileSystem({ 'index.js': '', aPackage: { @@ -1807,10 +1812,9 @@ function dep(name: string): TransformResultDependency { }, }); - await expect(createResolver(config)).rejects.toThrow( - 'Duplicated files or mocks. Please check the console for more info', - ); - expect(console.error).toHaveBeenCalledWith( + resolver = await createResolver(config); + + expect(console.warn).toHaveBeenCalledWith( [ 'metro-file-map: Haste module naming collision: aPackage', ' The following files share their name; please adjust your hasteImpl:', @@ -1823,9 +1827,26 @@ function dep(name: string): TransformResultDependency { '', ].join('\n'), ); + + expect(() => + resolver.resolve(p('/root/index.js'), dep('aPackage')), + ).toThrowError( + new AmbiguousModuleResolutionError( + p('/root/index.js'), + new DuplicateHasteCandidatesError( + 'aPackage', + Haste.GENERIC_PLATFORM, + false, + new Map([ + [p('/root/aPackage/package.json'), Haste.PACKAGE], + [p('/root/anotherPackage/package.json'), Haste.PACKAGE], + ]), + ), + ).message, + ); }); - test('does not support multiple global packages for different platforms', async () => { + test('resolution throws on multiple global packages for different platforms', async () => { setMockFileSystem({ 'index.js': '', 'aPackage.android.js': { @@ -1838,10 +1859,9 @@ function dep(name: string): TransformResultDependency { }, }); - await expect(createResolver(config)).rejects.toThrow( - 'Duplicated files or mocks. Please check the console for more info', - ); - expect(console.error).toHaveBeenCalledWith( + resolver = await createResolver(config, 'ios'); + + expect(console.warn).toHaveBeenCalledWith( [ 'metro-file-map: Haste module naming collision: aPackage', ' The following files share their name; please adjust your hasteImpl:', @@ -1858,6 +1878,23 @@ function dep(name: string): TransformResultDependency { '', ].join('\n'), ); + + expect(() => + resolver.resolve(p('/root/index.js'), dep('aPackage')), + ).toThrowError( + new AmbiguousModuleResolutionError( + p('/root/index.js'), + new DuplicateHasteCandidatesError( + 'aPackage', + Haste.GENERIC_PLATFORM, + false, + new Map([ + [p('/root/aPackage.android.js/package.json'), Haste.PACKAGE], + [p('/root/aPackage.ios.js/package.json'), Haste.PACKAGE], + ]), + ), + ).message, + ); }); test('resolves global packages before node_modules packages', async () => { @@ -2101,17 +2138,16 @@ function dep(name: string): TransformResultDependency { }); }); - test('fatals when there are duplicated haste names', async () => { + test('resolution throws when there are duplicated haste names', async () => { setMockFileSystem({ 'index.js': '', 'hasteModule.js': '@providesModule hasteModule', 'anotherHasteModule.js': '@providesModule hasteModule', }); - await expect(createResolver(config)).rejects.toThrow( - 'Duplicated files or mocks. Please check the console for more info', - ); - expect(console.error).toHaveBeenCalledWith( + resolver = await createResolver(config); + + expect(console.warn).toHaveBeenCalledWith( [ 'metro-file-map: Haste module naming collision: hasteModule', ' The following files share their name; please adjust your hasteImpl:', @@ -2120,6 +2156,23 @@ function dep(name: string): TransformResultDependency { '', ].join('\n'), ); + + expect(() => + resolver.resolve(p('/root/index.js'), dep('hasteModule')), + ).toThrowError( + new AmbiguousModuleResolutionError( + p('/root/index.js'), + new DuplicateHasteCandidatesError( + 'hasteModule', + Haste.GENERIC_PLATFORM, + false, + new Map([ + [p('/root/anotherHasteModule.js'), Haste.MODULE], + [p('/root/hasteModule.js'), Haste.MODULE], + ]), + ), + ).message, + ); }); test('resolves a haste module before a package in node_modules', async () => { @@ -2143,7 +2196,7 @@ function dep(name: string): TransformResultDependency { }); }); - test('fatals when a haste module collides with a global package', async () => { + test('resolution throws when a haste module collides with a global package', async () => { setMockFileSystem({ 'index.js': '', 'hasteModule.js': '@providesModule hasteModule', @@ -2152,10 +2205,9 @@ function dep(name: string): TransformResultDependency { }, }); - await expect(createResolver(config)).rejects.toThrow( - 'Duplicated files or mocks. Please check the console for more info', - ); - expect(console.error).toHaveBeenCalledWith( + resolver = await createResolver(config); + + expect(console.warn).toHaveBeenCalledWith( [ 'metro-file-map: Haste module naming collision: hasteModule', ' The following files share their name; please adjust your hasteImpl:', @@ -2164,6 +2216,23 @@ function dep(name: string): TransformResultDependency { '', ].join('\n'), ); + + expect(() => + resolver.resolve(p('/root/index.js'), dep('hasteModule')), + ).toThrowError( + new AmbiguousModuleResolutionError( + p('/root/index.js'), + new DuplicateHasteCandidatesError( + 'hasteModule', + Haste.GENERIC_PLATFORM, + false, + new Map([ + [p('/root/aPackage/package.json'), Haste.PACKAGE], + [p('/root/hasteModule.js'), Haste.MODULE], + ]), + ), + ).message, + ); }); test('supports collisions between haste names and global packages if they have different platforms', async () => { @@ -2215,17 +2284,16 @@ function dep(name: string): TransformResultDependency { }); }); - test('fatals when a filename uses a non-supported platform and there are collisions', async () => { + test('warns when a filename uses a non-supported platform and there are collisions', async () => { setMockFileSystem({ 'index.js': '', 'hasteModule.js': '@providesModule hasteModule', 'hasteModule.invalid.js': '@providesModule hasteModule', }); - await expect(createResolver(config)).rejects.toThrow( - 'Duplicated files or mocks. Please check the console for more info', - ); - expect(console.error).toHaveBeenCalledWith( + resolver = await createResolver(config); + + expect(console.warn).toHaveBeenCalledWith( [ 'metro-file-map: Haste module naming collision: hasteModule', ' The following files share their name; please adjust your hasteImpl:', diff --git a/packages/metro/src/node-haste/DependencyGraph.js b/packages/metro/src/node-haste/DependencyGraph.js index 1ff5a9fa64..e8158fc3f3 100644 --- a/packages/metro/src/node-haste/DependencyGraph.js +++ b/packages/metro/src/node-haste/DependencyGraph.js @@ -101,7 +101,10 @@ class DependencyGraph extends EventEmitter { type: 'dep_graph_loading', hasReducedPerformance: !!hasReducedPerformance, }); - const fileMap = createFileMap(config, {watch}); + const fileMap = createFileMap(config, { + throwOnModuleCollision: false, + watch, + }); // We can have a lot of graphs listening to Haste for changes. // Bump this up to silence the max listeners EventEmitter warning.