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.

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

Fix recovery of haste collisions #7329

Merged
merged 3 commits into from
Nov 7, 2018

Conversation

rafeca
Copy link
Contributor

@rafeca rafeca commented Nov 5, 2018

Summary

jest-haste-map had a bug when recovering from a haste name collision: it was using the type from the deleted file instead of the type from the remaining file in the haste map.

This PR fixes this issue, while it also changes the internal data structure that holds the duplicated modules.

I've split the PR into two commits: the first one only fixes the issue and the second one converts the duplicates structure into a Map. We can discard the second commit if needed.

I've marked the change as a breaking change since the duplicated modules are part of the public API of jest-haste-map (although it should be very rare for a user to rely on this).

Test plan

I've created a new unit test that ensures the correct behaviour (that test would fail currently in master).

@rafeca rafeca requested a review from jeanlauliac November 5, 2018 16:00
@rafeca rafeca force-pushed the fix-recovery-of-haste-collisions branch 5 times, most recently from 8712cd5 to 16b094d Compare November 5, 2018 16:29
Copy link
Contributor

@jeanlauliac jeanlauliac left a comment

Choose a reason for hiding this comment

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

I remember we were using an object so that it can be serialised/deserialised from the cache very fast. A Map would not get serialised properly I think. Can you confirm what's the status for this?

@rafeca
Copy link
Contributor Author

rafeca commented Nov 6, 2018

I remember we were using an object so that it can be serialised/deserialised from the cache very fast. A Map would not get serialised properly I think. Can you confirm what's the status for this?

The cache system of jest-haste-map uses v8s serialize/deserialize methods, which support Maps.

Regarding performance, I wouldn't worry here since the number of duplicates in the haste map should be rather small

@rafeca rafeca dismissed jeanlauliac’s stale review November 6, 2018 20:12

Answered the concern that @jeanlauliac had

Copy link
Contributor

@jeanlauliac jeanlauliac left a comment

Choose a reason for hiding this comment

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

Cool, then that sounds good to me 🎉

@rafeca rafeca force-pushed the fix-recovery-of-haste-collisions branch from 16b094d to e2f21d4 Compare November 7, 2018 14:15
@rafeca rafeca merged commit fa0e814 into jestjs:master Nov 7, 2018
@rafeca rafeca deleted the fix-recovery-of-haste-collisions branch November 7, 2018 16:43
@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants