Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

jest-haste-map: watch-mode recover from duplicate modules #3107

Closed
wants to merge 1 commit into from
Closed

jest-haste-map: watch-mode recover from duplicate modules #3107

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 2 additions & 10 deletions packages/jest-haste-map/src/__tests__/__snapshots__/index-test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`HasteMap throws on duplicate module ids if "throwOnModuleCollision" is set to true 1`] = `
exports[`HasteMap throws on duplicate module ids 1`] = `
[Error: jest-haste-map: @providesModule naming collision:
Duplicate module name: Strawberry
Paths: /fruits/raspberry.js collides with /fruits/strawberry.js

This error is caused by a @providesModule declaration with the same name across two different files.]
This error is caused by a @providesModule declaration with the same name across several different files.]
`;

exports[`HasteMap tries to crawl using node as a fallback 1`] = `
Expand All @@ -27,11 +27,3 @@ Jest will use the mock file found in:

"
`;

exports[`HasteMap warns on duplicate module ids 1`] = `
"jest-haste-map: @providesModule naming collision:
Duplicate module name: Strawberry
Paths: /fruits/raspberry.js collides with /fruits/strawberry.js

This warning is caused by a @providesModule declaration with the same name across two different files."
`;
80 changes: 58 additions & 22 deletions packages/jest-haste-map/src/__tests__/index-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,7 +341,7 @@ describe('HasteMap', () => {
});
});

it('warns on duplicate module ids', () => {
it('throws on duplicate module ids', () => {
// Raspberry thinks it is a Strawberry
mockFs['/fruits/raspberry.js'] = [
'/**',
Expand All @@ -350,27 +350,7 @@ describe('HasteMap', () => {
'const Banana = require("Banana");',
].join('\n');

return new HasteMap(defaultConfig).build()
.then(({__hasteMapForTest: data}) => {
expect(data.map.Strawberry[H.GENERIC_PLATFORM][0])
.toEqual('/fruits/raspberry.js');

expect(console.warn.mock.calls[0][0]).toMatchSnapshot();
});
});

it('throws on duplicate module ids if "throwOnModuleCollision" is set to true', () => {
// Raspberry thinks it is a Strawberry
mockFs['/fruits/raspberry.js'] = [
'/**',
' * @providesModule Strawberry',
' */',
'const Banana = require("Banana");',
].join('\n');

return new HasteMap(
Object.assign({throwOnModuleCollision: true}, defaultConfig),
).build().catch(err => {
return new HasteMap(defaultConfig).build().catch(err => {
expect(err).toMatchSnapshot();
});
});
Expand Down Expand Up @@ -815,6 +795,62 @@ describe('HasteMap', () => {
}),
);
},
() => {
mockFs['/fruits/pear.js'] = [
'/**',
' * @providesModule Pear',
' */',
].join('\n');
mockFs['/fruits/blueberry.js'] = [
'/**',
' * @providesModule Pear',
' */',
].join('\n');

mockEmitters['/fruits'].emit(
'all',
'change',
'pear.js',
'/fruits',
statObject,
);
mockEmitters['/fruits'].emit(
'all',
'add',
'blueberry.js',
'/fruits',
statObject,
);

hasteMap.once(
'change',
addErrorHandler(({eventsQueue, hasteFS, moduleMap}) => {
expect(moduleMap.getModule('Pear')).toBe(null);
expect(hasteFS.exists('/fruits/blueberry.js')).toBe(true);

mockFs['/fruits/blueberry.js'] = [
'/**',
' * @providesModule Blueberry',
' */',
].join('\n');
mockEmitters['/fruits'].emit(
'all',
'change',
'blueberry.js',
'/fruits',
statObject,
);
hasteMap.once(
'change',
addErrorHandler(({eventsQueue, hasteFS, moduleMap}) => {
expect(moduleMap.getModule('Pear')).toBe("/fruits/pear.js");
expect(moduleMap.getModule('Blueberry')).toBe("/fruits/blueberry.js");
next();
}),
);
}),
);
}
];

next();
Expand Down
105 changes: 78 additions & 27 deletions packages/jest-haste-map/src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,6 @@ type Options = {
resetCache?: boolean,
retainAllFiles: boolean,
roots: Array<string>,
throwOnModuleCollision?: boolean,
useWatchman?: boolean,
watch?: boolean,
};
Expand All @@ -70,7 +69,6 @@ type InternalOptions = {
resetCache: ?boolean,
retainAllFiles: boolean,
roots: Array<string>,
throwOnModuleCollision: boolean,
useWatchman: boolean,
watch: boolean,
};
Expand Down Expand Up @@ -215,7 +213,6 @@ class HasteMap extends EventEmitter {
resetCache: options.resetCache,
retainAllFiles: options.retainAllFiles,
roots: Array.from(new Set(options.roots)),
throwOnModuleCollision: !!options.throwOnModuleCollision,
useWatchman: options.useWatchman == null ? true : options.useWatchman,
watch: !!options.watch,
};
Expand All @@ -234,6 +231,8 @@ class HasteMap extends EventEmitter {
this._workerPromise = null;
this._workerFarm = null;
this._watchers = [];
this._moduleIDsByFilePath = new Map();
this._moduleArraysByIDAndPlatform = new Map();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm being super annoying I'd ask you to sort these alphabetically within the block.

}

static getCacheFilePath(tmpdir: Path, name: string): string {
Expand Down Expand Up @@ -290,6 +289,9 @@ class HasteMap extends EventEmitter {
.then(hasteMap => this._crawl(hasteMap));
}

_moduleIDsByFilePath: Map<string, string>;
_moduleArraysByIDAndPlatform: Map<string, Map<string, Array<ModuleMetaData>>>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move these to the top of the file?


/**
* 3. parse and extract metadata from changed files.
*/
Expand All @@ -299,30 +301,66 @@ class HasteMap extends EventEmitter {
mocks: Object,
filePath: Path,
workerOptions: ?{forceInBand: boolean},
): ?Promise<void> {
const setModule = (id: string, module: ModuleMetaData) => {
): ?Promise<?{id: string, candidates: Array<ModuleMetaData>}> {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rm this empty line, prettier will remove it later on too :D

const updateModuleFromCandidates = (
id: string,
platform: string,
candidates: Array<ModuleMetaData>,
) => {
if (!map[id]) {
map[id] = Object.create(null);
}
const moduleMap = map[id];
const platform = getPlatformExtension(module[H.PATH]) ||
H.GENERIC_PLATFORM;
const existingModule = moduleMap[platform];
if (existingModule && existingModule[H.PATH] !== module[H.PATH]) {
const message = `jest-haste-map: @providesModule naming collision:\n` +
` Duplicate module name: ${id}\n` +
` Paths: ${module[H.PATH]} collides with ` +
`${existingModule[H.PATH]}\n\nThis ` +
`${this._options.throwOnModuleCollision ? 'error' : 'warning'} ` +
`is caused by a @providesModule declaration ` +
`with the same name across two different files.`;
if (this._options.throwOnModuleCollision) {
throw new Error(message);
const modulesByPlatform = map[id];
if (candidates.length === 1) {
modulesByPlatform[platform] = candidates[0];
} else {
delete modulesByPlatform[platform];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may make sense to add a comment here:

// If there are more than one candidate for the same module, we remove them from the map.

}
return {id, candidates};
}

const setModule = (id: string, module: ModuleMetaData) => {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm

const moduleFilePath = module[H.PATH];
const platform =
getPlatformExtension(moduleFilePath) || H.GENERIC_PLATFORM;

const existingID = this._moduleIDsByFilePath.get(moduleFilePath);
if (existingID != null) {
const moduleArraysByPlatform = this._moduleArraysByIDAndPlatform.get(existingID);
if (moduleArraysByPlatform == null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (!moduleArraysByPlatform)?

throw new Error('could not find data for existing ID');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these just invariants that will never happen or do you expect user code to run into this for some reason? If it is user code, then please prefix them with jest-haste-map: and if it is internal code, please prefix with invariant: . Regardless of which it is, please capitalize the first word and add a . at the end of the sentence :)

}
this._console.warn(message);
const modules = moduleArraysByPlatform.get(platform);
if (modules == null) {
throw new Error('could not find candidate modules for existing ID and platform');
}
const existingModuleIx =
modules.findIndex(m => m[H.PATH] == moduleFilePath);
if (existingModuleIx < 0) {
throw new Error('could not find module for existing ID, platform and path');
}
modules.splice(existingModuleIx, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fancy. Why can we not use a Set for this?

updateModuleFromCandidates(existingID, platform, modules);
}

this._moduleIDsByFilePath.set(moduleFilePath, id);

let moduleArraysByPlatform = this._moduleArraysByIDAndPlatform.get(id);
if (moduleArraysByPlatform == null) {
moduleArraysByPlatform = new Map();
this._moduleArraysByIDAndPlatform.set(id, moduleArraysByPlatform);
}

moduleMap[platform] = module;
let modules = moduleArraysByPlatform.get(platform);
if (modules == null) {
moduleArraysByPlatform.set(platform, modules = []);
}

modules.push(module);
return updateModuleFromCandidates(id, platform, modules);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rm

};

// If we retain all files in the virtual HasteFS representation, we avoid
Expand Down Expand Up @@ -366,15 +404,17 @@ class HasteMap extends EventEmitter {
hasteImplModulePath: this._options.hasteImplModulePath,
}).then(
metadata => {
let processingResult = null;
// `1` for truthy values instead of `true` to save cache space.
fileMetadata[H.VISITED] = 1;
const metadataId = metadata.id;
const metadataModule = metadata.module;
if (metadataId && metadataModule) {
fileMetadata[H.ID] = metadataId;
setModule(metadataId, metadataModule);
processingResult = setModule(metadataId, metadataModule);
}
fileMetadata[H.DEPENDENCIES] = metadata.dependencies || [];
return processingResult;
},
error => {
// If a file cannot be read we remove it from the file list and
Expand All @@ -391,9 +431,22 @@ class HasteMap extends EventEmitter {

for (const filePath in hasteMap.files) {
const promise = this._processFile(hasteMap, map, mocks, filePath);
if (promise) {
promises.push(promise);
if (!promise) {
continue;
}
promises.push(promise.then(result => {
if (result == null || result.candidates.length <= 1) {
return;
}
throw new Error(
`jest-haste-map: @providesModule naming collision:\n` +
` Duplicate module name: ${result.id}\n` +
` Paths: ${result.candidates[1][H.PATH]} collides with ` +
`${result.candidates[0][H.PATH]}\n\nThis error ` +
`is caused by a @providesModule declaration ` +
`with the same name across several different files.`,
);
}));
}

const cleanup = () => {
Expand Down Expand Up @@ -528,9 +581,7 @@ class HasteMap extends EventEmitter {
return Promise.resolve();
}

// 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;
// In watch mode, we'll retain all files, even changes to node_modules.
this._options.retainAllFiles = true;

const Watcher = canUseWatchman && this._options.useWatchman
Expand Down