-
-
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
jest-haste-map: Emit change events if a file in node_modules has changed #2682
Conversation
@@ -638,6 +640,9 @@ class HasteMap extends EventEmitter { | |||
this._workerPromise = null; | |||
if (promise) { | |||
return promise.then(add); | |||
} else { | |||
// If a file in node_modules has changed, emit an event regardless. | |||
add(); | |||
} | |||
} else { |
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.
you could also remove this else and always add before the return null, I feel your way is better to follow the logic though.
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.
No, we need to wait for the promise if processFile returns one.
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.
That's done by the if, what I was thinking is to remove the else
condition you just add and the outer else
one below and just add add()
before return null
.
if type is not add
nor change
you were calling add
anyway and now with your change you are not calling it only in the case processFile returns a promise.
As I said I prefer your implementation for readability.
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.
Oh, got it. I prefer this to be more explicit because the haste map code is really complex.
Generated by 🚫 dangerJS |
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
Fixes #2592 and will fix facebook/react-native#11301 once Jest 19 is out.
Explanation: since we don't store the haste map in watch mode, it is ok to "retain all files". This will issue change events when files in node_modules have changed, which will make the watch mode in Jest 19 behave more like Jest 18 (where changes to node_modules would re-run Jest), as well as fix something in react-native-packager.
Related, I kinda feel like retainAllFiles has to go. It makes things faster on our giant codebase but gives a bunch of trouble.
Test plan