jest-haste-map: recover from duplicate IDs #3647

jeanlauliac · 2017-05-24T11:21:41Z

This supersedes #3107. This changeset adds a new top-level structure to the "HasteMap" object kept internally. This structure keeps track of duplicated modules. We cannot keep track of these in the existing map field, that accepts only a single resolution per module name/ID. An alternative solution would have been to change the structure of map, for example to keep an array of file paths for each module name/ID. However, this would incur significant overhead persisting the whole "HasteMap". By adding a separate index structure, that only contains the strict necessary, we ensure minimum overhead is added for persistence.

Test plan

Unit tests. As you can see, I fixed the unit tests that were testing the broken behavior, that is now correct.

jeanlauliac · 2017-05-24T11:44:50Z

packages/jest-haste-map/src/index.js

+        }
+        let dupsByPl = hasteMap.duplicates[id];
+        if (dupsByPl == null) {
+          dupsByPl = hasteMap.duplicates[id] = (Object.create(null): any);


these any are necessary, otherwise Flow complains below that is cannot find "g" (special case of "platform") in the object. The problem is that we're precisely assigning "g", not trying to fetch it, so that's a little weird. I think it's a false positive from Flow.

cpojer · 2017-05-24T11:46:23Z

packages/jest-haste-map/src/__tests__/index-test.js

-        expect(data.map.Strawberry[H.GENERIC_PLATFORM][0]).toEqual(
-          '/fruits/raspberry.js',
-        );
+        // duplicate modules are removed so that it doesn't cause


Comments are sentences, please capitalize the first character :)

cpojer · 2017-05-24T11:48:13Z

packages/jest-haste-map/src/index.js

@@ -325,6 +325,28 @@ class HasteMap extends EventEmitter {
          throw new Error(message);
        }
        this._console.warn(message);


Should we remove this warning (Can be done in a follow-up PR)? It doesn't provide much use.

I disagree, we need to leave it so that people know there's duplication during watchmode. Without it it would say "module doesn't exist", without any actionable feedback.

What we can do later is improve the API so that consumers can identify duplicated IDs.

Oh, yeah, I think in the initial PR the way it was implemented is that we moved this error handling up the stack so that consumers can decide what to do it. We may want to do that because otherwise, in RN, we won't be able to show a redbox. Most people don't look at the cli output during development. However, this can of course be done later.

Let's do it as a future step, as this changeset make it no worse than the current situation. I definitely think doing "console.warn/log" in the middle of a library is an anti-pattern and that we should expose it as an error condition through getModule instead.

cpojer · 2017-05-24T11:48:53Z

packages/jest-haste-map/src/index.js

+        if (Object.keys(moduleMap).length === 1) {
+          delete map[id];
+        }
+        let dupsByPl = hasteMap.duplicates[id];


mind spelling out dupsByPlatform? Personally I'd prefer duplicatesByPlatform but I think dups is not ambiguous, so it is fine,

cpojer · 2017-05-24T11:51:42Z

packages/jest-haste-map/src/index.js

+        let dupsByPl = hasteMap.duplicates[id];
+        if (dupsByPl == null) {
+          dupsByPl = hasteMap.duplicates[id] = (Object.create(null): any);
+        }


You could turn this into const, which is imho more readable, but up to you:

const dupsByPl = hasteMap.duplicates[id] == null ? (hasteMap.duplicates[id] = (Object.create(null): any)) : hasteMap.duplicates[id];

I guess it only matters if you are trying to avoid let :)

Yeah but then it does 2 key accesses instead of one for no clear benefit.

Ah, now I get it. This is because at that point the original module won't be part of the moduleMap, so the previous if-statement with existingModule && existingModule[H.PATH] !== module[H.PATH] will be false.

cpojer · 2017-05-24T12:02:28Z

packages/jest-haste-map/src/index.js

+        if (dups != null) {
+          dups[module[H.PATH]] = module[H.TYPE];
+        }
+        return;


I'm not sure I understand the purpose of this block. Can you elaborate on why we need lines 343-349? As I understand it, if an id with a collision has yet another collision, it will overwrite whatever is currently in the duplicates map for that platform. Why?

This block checks if there's already duplicates for that name yeah (in which case there are minimum 2 elements in dups already). We need to add the file we're processing to the list of duplicates for that name. We are not overwriting the others here, we are adding it to the 'set'.

cpojer · 2017-05-24T12:17:32Z

packages/jest-haste-map/src/index.js

+    dupsByPl = hasteMap.duplicates[moduleName] = (copy(dupsByPl): any);
+    dups = dupsByPl[platform] = (copy(dups): any);
+    const dedupType = dups[filePath];
+    delete dups[filePath];


Alright, the remaining thing I'm confused about is why the currently changed file gets removed from the duplicate map on this line.

Look at the code above the callsite of _recoverDuplicates. What we do is that we remove all references to the changed code, both in the hasteMap.files index and in the hasteMap.map, so that when we process it we know we just have to insert it, there's nothing else to remove elsewhere.

Here is the same idea. Basically as soon as a file changed, whether deleted/changed, we remove it from the duplication index as if it had been deleted. Later on, we run the file-processing code above, that will re-create the duplication index for that file in case the ID conflict still happens. Of course that means if a conflicted file is changed again and again, we'll delete and re-create the duplication index for its ID again and again. But, this makes the logic simpler and more consistent with the rest of the jest-haste-map architecture. We could implement a complicated logic that checks if the ID changed, and delete the file from the duplication index only if it did change. But, I believe that logic would be more complex, and more importantly, more fragile. It'd also be harder to implement given the current structure of the system ^_^

Does that answer the question?

Yes! Thank you, that totally makes sense. Nice work!

* jest-haste-map: do not expose any module when ID is duplicate * jest-haste-map: build up an index of duplicate IDs * jest-haste-map: recover from duplicate module IDs * jest-haste-map: fix prettier * fix all tests+flow * jest-haste-map: address comments

github-actions · 2021-05-13T15:06:50Z

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

Jean Lauliac added 3 commits May 24, 2017 09:52

jest-haste-map: do not expose any module when ID is duplicate

7a1d03a

jest-haste-map: build up an index of duplicate IDs

9bd09d9

jest-haste-map: recover from duplicate module IDs

8bbd0ca

jeanlauliac requested a review from cpojer May 24, 2017 11:21

facebook-github-bot added the CLA Signed ✔️ label May 24, 2017

Jean Lauliac added 2 commits May 24, 2017 12:22

jest-haste-map: fix prettier

6e42298

fix all tests+flow

5cd00a3

jeanlauliac commented May 24, 2017

View reviewed changes

cpojer reviewed May 24, 2017

View reviewed changes

jest-haste-map: address comments

9d354dd

cpojer merged commit 02e20c9 into jestjs:master May 24, 2017

renovate-bot mentioned this pull request May 24, 2017

Update dependency jest to version 20.0.4 renovatebot/renovate#208

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

jest-haste-map: recover from duplicate IDs #3647

jest-haste-map: recover from duplicate IDs #3647

jeanlauliac commented May 24, 2017

jeanlauliac May 24, 2017 •

edited

Loading

cpojer May 24, 2017

cpojer May 24, 2017

jeanlauliac May 24, 2017

cpojer May 24, 2017

jeanlauliac May 24, 2017

cpojer May 24, 2017

cpojer May 24, 2017

jeanlauliac May 24, 2017

cpojer May 24, 2017

cpojer May 24, 2017

jeanlauliac May 24, 2017 •

edited

Loading

cpojer May 24, 2017

jeanlauliac May 24, 2017 •

edited

Loading

cpojer May 24, 2017

github-actions bot commented May 13, 2021

jest-haste-map: recover from duplicate IDs #3647

jest-haste-map: recover from duplicate IDs #3647

Conversation

jeanlauliac commented May 24, 2017

jeanlauliac May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanlauliac May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeanlauliac May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented May 13, 2021

jeanlauliac May 24, 2017 •

edited

Loading

jeanlauliac May 24, 2017 •

edited

Loading

jeanlauliac May 24, 2017 •

edited

Loading