Skip to content

Commit

Permalink
Don't zombie on preexisting Haste collisions on startup
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@robhogan @facebook-github-bot
robhogan authored and facebook-github-bot committed Dec 10, 2024
1 parent 44bab52 commit 0fc8e45
Show file tree
Hide file tree
Showing 2 changed files with 100 additions and 29 deletions.
124 changes: 96 additions & 28 deletions packages/metro/src/DeltaBundler/__tests__/resolver-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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);

Expand Down Expand Up @@ -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: {
Expand All @@ -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:',
Expand All @@ -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': {
Expand All @@ -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:',
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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:',
Expand All @@ -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 () => {
Expand All @@ -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',
Expand All @@ -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:',
Expand All @@ -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 () => {
Expand Down Expand Up @@ -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:',
Expand Down
5 changes: 4 additions & 1 deletion packages/metro/src/node-haste/DependencyGraph.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down

0 comments on commit 0fc8e45

Please sign in to comment.