Repository navigation
react-scripts test is watching files in node_modules #2393
Description
Can you reproduce the problem with latest npm?
yes
Can you still reproduce it?
yes, also did npm cache clean just to be sure
Description
This is a followup of "npm test hangs for newly generated app (#960)". After reading the comment of @hmeerlo that "Jest is trying to watch too many files at once (jestjs/jest#1767 (comment))" I suspect that files in node_modules are unnecessarily being watched.
Expected behavior
npm t should run without error.
Actual behavior
react-scripts test --env=jsdom
2017-05-28 09:18 node[1154] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
2017-05-28 09:18 node[1154] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
events.js:160
throw er; // Unhandled 'error' event
^
Error: Error watching file for changes: EMFILE
at exports._errnoException (util.js:1018:11)
at FSEvent.FSWatcher._handle.onchange (fs.js:1420:11)
npm ERR! Test failed. See above for more details.
Environment
Run these commands in the project folder and fill in their results:
npm ls react-scripts(if you haven’t ejected): react-scripts@1.0.7node -v: v6.10.3npm -v: 4.6.1
Then, specify:
- Operating system: macOS Sierra 10.12.5
- Browser and version: -
watchmannever installed
Reproducible Demo
https://github.com/thisconnect/cra-desktop
Reproduce the error
This is an instantiated create-react-app (un-ejected) with electron added as described in PR #1718
git clone https://github.com/thisconnect/cra-desktop
cd cra-desktop
npm install
npm test
Without error (when removing 2 large dependencies)
Assuming react-scripts test is indeed watching files in node_modules, remove electron and electron-packager from devDependencies in package.json, then rm -rf node_modules && npm i && npm t, test mode runs fine. This does not yet prove that files in node_modules are being watched.
Strong indication that node_modules is being watched:
- run any CRA instance without error
- the Terminal should display something like "No tests found related to files changed since last commit" and the "Watch Usage"
- open a random .js file in node_modules
- for example node_modules/mime/mime.js
- watch the Terminal (screenshot 1)
- add a comment on any line i.e.
// test - save the file
- check if the terminal changed (screenshot 2)
Terminal before changing a random file in node_modules (screenshot 1)
Terminal after chaning a random file in node_modules (screenshot 2)
Activity
thisconnect commentedon May 28, 2017
Side note: The error does not appear in
react-scripts@0.9.5and was introduced withreact-scripts@1.0.0thisconnect commentedon May 28, 2017
I am not exactly sure what the corresponding jest config is, maybe
testMatchin https://github.com/facebookincubator/create-react-app/blob/master/packages/react-scripts/scripts/utils/createJestConfig.js ?My test about
react-scripts testwatching seems to not only affectnode_modules/**/*.js, but also files in/dist/*.jsand in/build/*.js.ro-savage commentedon May 28, 2017
Thanks for creating the example source @thisconnect - I have a PR #2395 that should fix this issue.
Although the underlining issue is with Jest, and will depend if the maintainers of CRA would prefer to wait for Jest to fix, or fix it here until it's fixed in Jest.
gaearon commentedon May 28, 2017
How hard is it to fix in Jest?
ro-savage commentedon May 28, 2017
I am really unsure.
As mentioned in jestjs/jest/issues/1767 when Jest is using the node_watcher it is tells it to watch everything with chosen extensions, rather than just the files in
testMatch.Its possible the fix is just to pass
testMatchto the file watcher. Its also possible this breaks a bunch of other stuff. Unfortunately I don't understand why Jest does what it does. It might just be a bug or if there is some greater reason for it.Hopefully @cpojer or @pugnascotia will have a better answer.
cpojer commentedon May 30, 2017
@ro-savage fyi, if Jest only watches the files matching testMatch, that means that we won't be able to run tests when non-test files change. So that is the expected behavior.
ro-savage commentedon May 30, 2017
@cpojer - Of course, it didn't even click that it was only matching the test files.
Then it should be fine to use both
testMatchandmodulePathIgnorePatterns?In fact
modulePathIgnorePatternswould be the way correct to tell Jest what files/folders not to watch?(I understand there might be edge cases where you
npm installan updated module and then want the tests to rerun. But seems better to miss that edge case than crash.)gaearon commentedon May 30, 2017
This might help fix the crashes: amasad/sane#97.
I think we should first direct effort there, and if it doesn’t work out, try strategies in this issue.
thisconnect commentedon Jun 17, 2017
New sane version works for me amasad/sane#97 (comment)
thisconnect commentedon Jun 27, 2017
Jest merged sane in jestjs/jest#3918
jestjs/jest@acc783a
diptendulkar commentedon Jun 30, 2017
Reinstalling watchman fixed this error for me.
brew uninstall watchman
brew install watchman
thisconnect commentedon Aug 3, 2017
jest@20.1.0-alpha.3 should have the fixed
saneusingfseventsjestjs/jest#1767 (comment)
andrew-oxenburgh commentedon Aug 22, 2017
I did a
sudo port install watchman, and problem went away.Apparently you need watchman installed.
gaearon commentedon Jan 14, 2018
We updated to Jest 22 on the
nextbranch so while we're still watchingnode_modules, it should no longer cause the crash in this issue. We can't backport this toreact-scripts@1.xbut stay tuned for the alpha versions ofreact-scripts@2.xcoming soon.thisconnect commentedon Jan 14, 2018
thanks @gaearon !!!
Can I test
nextby installingfacebookincubator/create-react-app#next?gaearon commentedon Jan 14, 2018
No, we don’t support installation from GitHub. When it’s available, you will be able to just specify the version we release.
thisconnect commentedon Jan 14, 2018
ok, thanks
thisconnect commentedon May 1, 2018
@gaearon just FYI I am testing
react-scripts@2.0.0-next.66cc7a90(withoutwatchman) and it is actually the first timenpm testruns with the electron stuff 🎉 🎊 🎻 🏆 🎸 🍸 🎂 🌈 🚀 (test repo https://github.com/thisconnect/cra-desktop)Had a minor issue running
npm start, commented in #3815 (comment) and another issue with node6 travis CI https://travis-ci.org/thisconnect/cra-desktop/builds/373373379 something aboutRequires Babel "^7.0.0-0", but was loaded with "6.26.0".(travis node8 is fine)
I am sorry for commenting on this closed issue, just wanted to report and say thanks. Thanks!