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

Simplify and enforce duplicate haste map errors #8002

Merged
merged 1 commit into from
Feb 28, 2019
Merged

Simplify and enforce duplicate haste map errors #8002

merged 1 commit into from
Feb 28, 2019

Conversation

mjesun
Copy link
Contributor

@mjesun mjesun commented Feb 27, 2019

This PR changes the way duplicated mocks and haste names are reported. The current way allows to have multiple times the same name, but emits a warning. After two years of having the warning in, I can tell you no one looks at it; so this diff starts enforcing them to be unique.

I also took the opportunity to standardize and simplify error messaging, since it was nearly impossible to read.

Before:

jest-haste-map: duplicate manual mock found:
  Module name: subdir/Blueberry
  Duplicate Mock path: /project/fruits2/__mocks__/subdir/Blueberry.js
This warning is caused by two manual mock files with the same file name.
Jest will use the mock file found in:
/project/fruits2/__mocks__/subdir/Blueberry.js
 Please delete one of the following two files:
 /project/fruits1/__mocks__/subdir/Blueberry.js
/project/fruits2/__mocks__/subdir/Blueberry.js

After:

jest-haste-map: duplicate manual mock found: subdir/Blueberry
  The following files share their name; please delete one of them:
    * <rootDir>/fruits1/__mocks__/subdir/Blueberry.js
    * <rootDir>/fruits2/__mocks__/subdir/Blueberry.js

Tests were adjusted accordingly to reflect the new logging mechanisms.

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Should this be considered a breaking change?

@mjesun
Copy link
Contributor Author

mjesun commented Feb 27, 2019

Should this be considered a breaking change?

Probably, yes :)

@mjesun
Copy link
Contributor Author

mjesun commented Feb 27, 2019

Should this be considered a breaking change?
Probably, yes :)

Let me clarify my "probably, yes"; although it's technically a breaking change, I do not expect anyone from being affected by this (no one has two http or fs mocks at the same time in their codebase). The reason why this is so frequent for us is due to Haste, which is (almost) not used in OSS.

So I think that we could still release as minor.

@rubennorte
Copy link
Contributor

rubennorte commented Feb 27, 2019

@mjesun #2070 might make this a problem for general users as well, not only for those using haste, which is why I think this should be a breaking change and probably delayed until we're close to release a major. But we can do now instead, is adding a new option in config.haste to throw an error instead of a warning in these cases (e.g. config.haste.throwOnDuplicates being false by default). We could remove the option in the next major if we want that to be the only behaviour.

@mjesun
Copy link
Contributor Author

mjesun commented Feb 27, 2019

I will use throwOnModuleCollision to properly decide whether to break or not, so it's no longer a breaking change.

@rubennorte, you don't have to edit your comment to reflect what we just talked :D

@rubennorte
Copy link
Contributor

That would also be a breaking change but given it's change on an undocumented option I think we can just asumme it.

@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.

4 participants