Skip to content

Commit

Permalink
Fix Haste map cleanup with Haste module duplicates
Browse files Browse the repository at this point in the history 
Summary:
One issue led to another here -

1. I inadvertently broke [this condition](https://github.com/facebook/metro/blame/eeb211fdcfdcb9e7f8a51721bd0f48bc7d0d211f/packages/metro-file-map/src/index.js#L527) when refactoring the data structures in D42303308 (ef9af83) - to preserve the previous behaviour, `moduleMap` should've been changed to `moduleMapItem` on that line. The condition has since been a no-op, because `moduleMap` ([`RawModuleMap`](https://github.com/facebook/metro/blob/eeb211fdcfdcb9e7f8a51721bd0f48bc7d0d211f/packages/metro-file-map/src/flow-types.js#L232-L237)) always has 4 keys.
2. Thinking about fixing this led me to question what it does - in particular, why "`Object.keys(moduleMapItem).length === 1`" and not "`=== 0`" before deleting the entry from the `moduleMap`. It was introduced in jestjs/jest#3647 but there's no discussion of that line. I'm 99% certain it's meant to remove an empty entry from the module map, but obviously it actually removes an entry if and only if it has exactly 1 remaining platform.

For example, this would've meant that if three `Banana` Haste modules exist, with a duplicate for `ios`:

`Banana.js`
`Banana.ios.js`
`another/Banana.ios.js`

`jest-haste-map` would detect a duplicate `Banana.ios`, correctly delete `moduleMap.get('Banana')[ios]`, and then *also* delete the whole entry (along with the generic platform) *without* adding `Banana.js` to duplicates.

Then, if `another/Banana.ios.js` is deleted from disk to theoretically recover a good state, the Haste map would end up in a bad state, with no generic platform entry  `Banana.js`.

So in fact, D42303308 (ef9af83) accidentally fixed this bug by breaking the over-zealous deletion, but left some nonsense code. This diff restores the presumed original intent of the code, removing an empty object from the module map.

Changelog: [Internal]

Reviewed By: jacdebug

Differential Revision: D43664985

fbshipit-source-id: 61ae91f820bc6e2cf4985ae22c295d98df3da8a1
  • Loading branch information
@robhogan @facebook-github-bot
robhogan authored and facebook-github-bot committed Mar 2, 2023
1 parent cd25c2b commit 3cbd9ae
Show file tree
Hide file tree
Showing 2 changed files with 68 additions and 19 deletions.
80 changes: 65 additions & 15 deletions packages/metro-file-map/src/__tests__/index-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -784,9 +784,7 @@ describe('HasteMap', () => {

// Duplicate modules are removed so that it doesn't cause
// non-determinism later on.
expect(
cacheContent.map.get('Strawberry')[H.GENERIC_PLATFORM],
).not.toBeDefined();
expect(cacheContent.map.get('Strawberry')).not.toBeDefined();

expect(
console.warn.mock.calls[0][0].replaceAll('\\', '/'),
Expand Down Expand Up @@ -1046,6 +1044,14 @@ describe('HasteMap', () => {
expect(cacheContent.map.get('Strawberry')).toEqual({
g: [path.join('fruits', 'Strawberry.js'), 0],
});

delete mockFs[path.join('/', 'project', 'fruits', 'Strawberry.js')];
mockChangedFiles = object({
[path.join('/', 'project', 'fruits', 'Strawberry.js')]: null,
});
mockClocks = createMap({fruits: 'c:fake-clock:4'});
await new HasteMap(defaultConfig).build();
expect(cacheContent.map.get('Strawberry')).not.toBeDefined();
});

it('correctly handles platform-specific file renames', async () => {
Expand Down Expand Up @@ -1080,6 +1086,10 @@ describe('HasteMap', () => {
const Blackberry = require("Blackberry");
`;

mockFs[path.join('/', 'project', 'fruits', 'Banana.ios.js')] = '//';
mockFs[path.join('/', 'project', 'fruits', 'another', 'Banana.ios.js')] =
'//';

await new HasteMap(defaultConfig).build();
expect(deepNormalize(cacheContent.duplicates)).toEqual(
createMap({
Expand All @@ -1089,9 +1099,19 @@ describe('HasteMap', () => {
[path.join('fruits', 'another', 'Strawberry.js')]: H.MODULE,
}),
}),
Banana: createMap({
ios: createMap({
[path.join('fruits', 'Banana.ios.js')]: H.MODULE,
[path.join('fruits', 'another', 'Banana.ios.js')]: H.MODULE,
}),
}),
}),
);
expect(cacheContent.map.get('Strawberry')).toEqual({});
expect(cacheContent.map.get('Strawberry')).not.toBeDefined();

expect(cacheContent.map.get('Banana')).toBeDefined();
expect(cacheContent.map.get('Banana')[H.GENERIC_PLATFORM]).toBeDefined();
expect(cacheContent.map.get('Banana')['ios']).not.toBeDefined();
});

it('recovers when a duplicate file is deleted', async () => {
Expand All @@ -1107,7 +1127,9 @@ describe('HasteMap', () => {
});

await new HasteMap(defaultConfig).build();
expect(deepNormalize(cacheContent.duplicates)).toEqual(new Map());
expect(
deepNormalize(cacheContent.duplicates.get('Strawberry')),
).not.toBeDefined();
expect(cacheContent.map.get('Strawberry')).toEqual({
g: [path.join('fruits', 'Strawberry.js'), H.MODULE],
});
Expand All @@ -1117,6 +1139,32 @@ describe('HasteMap', () => {
});
});

it('recovers when a duplicate platform-specific file is deleted', async () => {
delete mockFs[
path.join('/', 'project', 'fruits', 'another', 'Banana.ios.js')
];
mockChangedFiles = object({
[path.join('/', 'project', 'fruits', 'another', 'Banana.ios.js')]: null,
});
mockClocks = createMap({
fruits: 'c:fake-clock:3',
vegetables: 'c:fake-clock:2',
});

await new HasteMap(defaultConfig).build();
expect(
deepNormalize(cacheContent.duplicates.get('Banana')),
).not.toBeDefined();
expect(cacheContent.map.get('Banana')).toEqual({
g: [path.join('fruits', 'Banana.js'), H.MODULE],
ios: [path.join('fruits', 'Banana.ios.js'), H.MODULE],
});
// Make sure the other files are not affected.
expect(cacheContent.map.get('Melon')).toEqual({
g: [path.join('vegetables', 'Melon.js'), H.MODULE],
});
});

it('recovers with the correct type when a duplicate file is deleted', async () => {
mockFs[
path.join('/', 'project', 'fruits', 'strawberryPackage', 'package.json')
Expand All @@ -1126,15 +1174,13 @@ describe('HasteMap', () => {

await new HasteMap(defaultConfig).build();

expect(deepNormalize(cacheContent.duplicates)).toEqual(
expect(deepNormalize(cacheContent.duplicates.get('Strawberry'))).toEqual(
createMap({
Strawberry: createMap({
g: createMap({
[path.join('fruits', 'Strawberry.js')]: H.MODULE,
[path.join('fruits', 'another', 'Strawberry.js')]: H.MODULE,
[path.join('fruits', 'strawberryPackage', 'package.json')]:
H.PACKAGE,
}),
g: createMap({
[path.join('fruits', 'Strawberry.js')]: H.MODULE,
[path.join('fruits', 'another', 'Strawberry.js')]: H.MODULE,
[path.join('fruits', 'strawberryPackage', 'package.json')]:
H.PACKAGE,
}),
}),
);
Expand Down Expand Up @@ -1162,7 +1208,9 @@ describe('HasteMap', () => {

await new HasteMap(defaultConfig).build();

expect(deepNormalize(cacheContent.duplicates)).toEqual(new Map());
expect(
deepNormalize(cacheContent.duplicates.get('Strawberry')),
).not.toBeDefined();
expect(cacheContent.map.get('Strawberry')).toEqual({
g: [path.join('fruits', 'Strawberry.js'), H.MODULE],
});
Expand All @@ -1181,7 +1229,9 @@ describe('HasteMap', () => {
});

await new HasteMap(defaultConfig).build();
expect(deepNormalize(cacheContent.duplicates)).toEqual(new Map());
expect(
deepNormalize(cacheContent.duplicates.get('Strawberry')),
).not.toBeDefined();
expect(cacheContent.map.get('Strawberry')).toEqual({
g: [path.join('fruits', 'Strawberry.js'), H.MODULE],
});
Expand Down
7 changes: 3 additions & 4 deletions packages/metro-file-map/src/index.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -524,7 +524,7 @@ export default class HasteMap extends EventEmitter {
// We do NOT want consumers to use a module that is ambiguous.
delete moduleMapItem[platform];
if (Object.keys(moduleMap).length === 1) {
if (Object.keys(moduleMapItem).length === 0) {
moduleMap.map.delete(id);
}
Expand Down Expand Up @@ -1143,11 +1143,10 @@ export default class HasteMap extends EventEmitter {
return;
}

let dedupMap = moduleMap.map.get(moduleName);
let dedupMap: ?ModuleMapItem = moduleMap.map.get(moduleName);

if (dedupMap == null) {
// $FlowFixMe[unclear-type] - ModuleMapItem?
dedupMap = (Object.create(null): any);
dedupMap = (Object.create(null): ModuleMapItem);
moduleMap.map.set(moduleName, dedupMap);
}
dedupMap[platform] = uniqueModule;
Expand Down

0 comments on commit 3cbd9ae

Please sign in to comment.