jest-haste-map: watch-mode recover from duplicate modules #3107
Conversation
…duplicate modules
There was a problem hiding this comment.
This is pretty great and a nice way to fix this problem altogether.
Would you mind adding a test that proves that a collision will also recover in the opposite case. From your test example that would be pear.js being changed to have a different name. It should make sure that blueberry.js will recover as "Pear" and pear.js will recover as something else.
The one concern I have is that we are now effectively duplicating some data from hasteFS/moduleMap which doubles memory consumption (~30mb, actually!). Could we change this to only keep track of more information for the items in the map that are candidates for the same module? The normal mode of operation is that everything is working fine and we should only kick in this new code when two modules actually conflict. What do you think?
| @@ -290,6 +289,9 @@ class HasteMap extends EventEmitter { | |||
| .then(hasteMap => this._crawl(hasteMap)); | |||
| } | |||
|
|
|||
| _moduleIDsByFilePath: Map<string, string>; | |||
| _moduleArraysByIDAndPlatform: Map<string, Map<string, Array<ModuleMetaData>>>; | |||
There was a problem hiding this comment.
Can you move these to the top of the file?
| @@ -234,6 +231,8 @@ class HasteMap extends EventEmitter { | |||
| this._workerPromise = null; | |||
| this._workerFarm = null; | |||
| this._watchers = []; | |||
| this._moduleIDsByFilePath = new Map(); | |||
| this._moduleArraysByIDAndPlatform = new Map(); | |||
There was a problem hiding this comment.
If I'm being super annoying I'd ask you to sort these alphabetically within the block.
| ): ?Promise<void> { | ||
| const setModule = (id: string, module: ModuleMetaData) => { | ||
| ): ?Promise<?{id: string, candidates: Array<ModuleMetaData>}> { | ||
|
|
There was a problem hiding this comment.
please rm this empty line, prettier will remove it later on too :D
| const existingID = this._moduleIDsByFilePath.get(moduleFilePath); | ||
| if (existingID != null) { | ||
| const moduleArraysByPlatform = this._moduleArraysByIDAndPlatform.get(existingID); | ||
| if (moduleArraysByPlatform == null) { |
| if (existingID != null) { | ||
| const moduleArraysByPlatform = this._moduleArraysByIDAndPlatform.get(existingID); | ||
| if (moduleArraysByPlatform == null) { | ||
| throw new Error('could not find data for existing ID'); |
There was a problem hiding this comment.
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 :)
| if (existingModuleIx < 0) { | ||
| throw new Error('could not find module for existing ID, platform and path'); | ||
| } | ||
| modules.splice(existingModuleIx, 1); |
There was a problem hiding this comment.
fancy. Why can we not use a Set for this?
|
|
||
| modules.push(module); | ||
| return updateModuleFromCandidates(id, platform, modules); | ||
|
|
| if (candidates.length === 1) { | ||
| modulesByPlatform[platform] = candidates[0]; | ||
| } else { | ||
| delete modulesByPlatform[platform]; |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| const setModule = (id: string, module: ModuleMetaData) => { | ||
|
|
Summary: This changeset adds a test that verifies that the duplicate modules use case is broken. The goal is to acknowledge the problem for now, and when we update `jest-haste`, we'll simply fix what needs to be fixed in that test. See also, jestjs/jest#3107 Reviewed By: davidaurelio Differential Revision: D4673431 fbshipit-source-id: 05e09bdf61a2eddf2e9d1e32a33d32065c9051f1
Summary: This changeset adds a test that verifies that the duplicate modules use case is broken. The goal is to acknowledge the problem for now, and when we update `jest-haste`, we'll simply fix what needs to be fixed in that test. See also, jestjs/jest#3107 Reviewed By: davidaurelio Differential Revision: D4673431 fbshipit-source-id: 05e09bdf61a2eddf2e9d1e32a33d32065c9051f1
|
This would need some extra love to get moving. I'll close for now because the rate at which eng. are getting this error is pretty low, and can be solved by clearing the cache. I'll focus on other priorities for the moment but I'm open to take this over some time later. I think |
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Also, remove the throwOnModuleCollision option.
Summary
When duplicate modules happen and are later fixed, the watch mode does not handle it properly. See the new unit test this PR adds. Without the changes, the test does not pass successfully.
I made the choice to remove duplicated modules from the
moduleMapaltogether. That avoids being able to resolve modules if there is any ambiguity, something that I think is the saner option ("fail fast" philosophy). Indeed, otherwise, people may be executing the wrong code without realising it.A problem that probably still need addressing in this PR is how duplicates are reporter to
jest-haste-mapconsumers. My proposal is that we could expose a third structure through thechangeevent that contains a list of duplicated IDs. (In my opinion, the best option would be to makegetModulereturn an array instead of a single element, and let consumers decide what to do about duplicates. But I understand this would be a breaking change, so I think the second-best option is a remove the ID form the map completely, and provide another mean for consumers to know of ID duplication.)Test plan
The automated tests demonstrate the fix.