-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Throw on module collision #1750
Conversation
…an error when a module name collision is detected (as opposed to simply printing a warning)
Current coverage is 87.41% (diff: 100%)@@ master #1750 diff @@
==========================================
Files 31 38 +7
Lines 1181 1200 +19
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
+ Hits 1036 1049 +13
- Misses 145 151 +6
Partials 0 0
|
@@ -273,6 +275,16 @@ class HasteMap { | |||
getPlatformExtension(module[H.PATH]) || H.GENERIC_PLATFORM; | |||
const existingModule = moduleMap[platform]; | |||
if (existingModule && existingModule[H.PATH] !== module[H.PATH]) { | |||
if (this._options.throwOnModuleCollision) { | |||
throw new Error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it looks like we are duplicating the entire message twice now with the only difference being error
and warning
. Can we instead assign the message to a variable only once and do one of these:
- Instead of
warning
anderror
, usemessage
. - Use
${this._options.throwOnModuleCollision ? 'error' : 'message'}
:)
@@ -49,6 +49,7 @@ type Options = { | |||
retainAllFiles: boolean, | |||
roots: Array<string>, | |||
useWatchman?: boolean, | |||
throwOnModuleCollision?: boolean, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please alphabetize the items in this list, we try to be consistent wherever we can :)
@@ -189,6 +190,7 @@ class HasteMap { | |||
roots: options.roots, | |||
useWatchman: | |||
options.useWatchman == null ? true : options.useWatchman, | |||
throwOnModuleCollision: options.throwOnModuleCollision, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
alphabetize here too within the context of this._options
:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this option is optional, we should do !!options.throwOnModuleCollision
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few minor nits to clean up the code a tiny bit.
nice work! |
* add throwOnModuleCollision option that, when set to true, will throw an error when a module name collision is detected (as opposed to simply printing a warning) * fix lint errors * alphabetize options; made code more compact * Update index.js * Update index-test.js.snap
* add throwOnModuleCollision option that, when set to true, will throw an error when a module name collision is detected (as opposed to simply printing a warning) * fix lint errors * alphabetize options; made code more compact * Update index.js * Update index-test.js.snap
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. |
Summary
Add
throwOnModuleCollision
option tojest-haste-map
. Setting this totrue
will cause it to throw an error (rather than output a warning to console) when a module name collision is detected. This is useful for scenarios where determinism is critical.Test plan
npm test