Fix Jest crashing on projects with large node_modules folders #2395

ro-savage · 2017-05-28T08:46:22Z

This PR fixes an ongoing issue with large node_modules folders and create-react-app tests.

The underlining issue is with Jest and how it watches files, and the current method that is used for testMatches means that files in node_modules are watched and crashes jest.

@thisconnect created issue #2393 that shows how to reproduce the error and included an example repo.

This PR uses modulePathIgnorePatterns to decide what to watch/not-watch instead of testMatches and correctly ignores node_modules. As seen in Jest issue jestjs/jest/issues/1767

thisconnect · 2017-05-28T08:53:50Z

Wow that was quick, thanks a lot!

thisconnect · 2017-05-28T08:54:39Z

packages/react-scripts/scripts/utils/createJestConfig.js

-      '<rootDir>/src/**/__tests__/**/*.js?(x)',
-      '<rootDir>/src/**/?(*.)(spec|test).js?(x)',
+    modulePathIgnorePatterns: [
+      '<rootDir>[/\\\\](build|docs|node_modules|scripts)[/\\\\]',


Would it be possible to add dist as well?

Thinking more about it in a perfect world everything in .gitignore should go here... coverage etc.

I've added dist as it's a common folder.
.gitignore really focuses on something different even if 99% of the time it's the same.

Its better to accidentally watch a folder that doesn't need to be watched than to accidentally not-watch a folder that should be watched.

Yes that makes sense thanks again!

thisconnect · 2017-05-28T09:02:56Z

packages/react-scripts/scripts/utils/createJestConfig.js

-      '<rootDir>/src/**/__tests__/**/*.js?(x)',
-      '<rootDir>/src/**/?(*.)(spec|test).js?(x)',
+    modulePathIgnorePatterns: [
+      '<rootDir>[/\\\\](build|dist|docs|node_modules|scripts)[/\\\\]',


Thanks! What about coverage that is created when npm run coverage is called

I guess nobody is watching and running coverage at the same time, ignore this comment

No harm adding it. But we can't cover everything. The main purpose for this really is to not watch node_modules as its too large and crashes Jest.

ro-savage · 2017-05-28T09:39:34Z

For future reference. PR #1614 added testMatches and PR #1808 updated the regex.

Once the underlining issue is solved at jestjs/jest#1767 we can revert back to testMatches.

The Regex at the time of this PR was

testMatches: [
  '<rootDir>/src/**/__tests__/**/*.js?(x)', 
  '<rootDir>/src/**/?(*.)(spec|test).js?(x)',
]

gaearon · 2017-05-28T10:17:40Z

@cpojer Is modulePathIgnorePatterns going away any time soon? Don’t want to rely on it if it’s deprecated or something.

cpojer · 2017-05-28T10:27:20Z

No plans to remove it.

cpojer · 2017-05-28T10:28:22Z

Note that when people try to debug something by changing files inside of node_modules, Jest will not re-run tests with this change applied.

gaearon · 2017-05-28T10:43:37Z

Fair enough, but better than instacrashing 😄
Thanks!

gaearon · 2017-05-28T10:45:53Z

packages/react-scripts/scripts/utils/createJestConfig.js

+    // Once this is fixes we can change from `modulePathIgnorePatterns` to `testMatches`.
+    // See facebook/jest/issues/1767 & facebookincubator/create-react-app/pull/2395
+    modulePathIgnorePatterns: [
+      '<rootDir>[/\\\\](build|dist|docs|node_modules|coverage|scripts|bower_components)[/\\\\]',


Seems like this still includes any arbitrary folders other than these? We specifically fixed it in 1.0 and I don't want to regress on it. Can we use a negative lookahead to ignore everything but src?

I believe this should work, but every time I regex I am worried I'll miss something.

<rootDir>\/(?!src).*\/.*

http://regexr.com/3g21u

I did a couple tests locally and it worked fine.

…g jest

thisconnect · 2017-05-28T15:16:15Z

packages/react-scripts/scripts/utils/createJestConfig.js

+    // Which causes crashes on large projects on MacOS.
+    // Once this is fixes we can change from `modulePathIgnorePatterns` to `testMatches`.
+    // See facebook/jest/issues/1767 & facebookincubator/create-react-app/pull/2395
+    modulePathIgnorePatterns: ['<rootDir>\/(?!src).*\/.*'],


Just tested, works for me. Thanks again

Timer · 2017-05-29T15:03:29Z

@thisconnect does installing watchman fix the problem?

It's a little inconvenient to switch away from testMatches because you can no longer edit node_modules/ and expect tests to re-run (which is nice!).

~~Can you run ulimit -n? Does it happen to be a low value? Could you try increasing it?~~

Cloning your repo on my macOS system does reproduce for me, but only after I uninstall watchman.
I agree this is a little painful, but is installing watchman too much to ask for large projects? 😄

Maybe we could recommend this in CRA, or Jest could and then handle EMFILE gracefully.

Does Windows have this problem?

gaearon · 2017-05-29T15:06:35Z

The watchman situation is really unfortunate. We should either make it a hard dependency on macOS or make things work without it.

Windows doesn’t have this problem (and watchman doesn’t work there anyway).

gaearon · 2017-05-29T15:11:01Z

@Timer We could use some of your digging in skills in figuring out what's the best place to fix it.

thisconnect · 2017-05-29T16:00:19Z

I have never installed watchman nor now anything about it. However watching node_modules/* just feels very wrong to me, isn't there any other measurable performance impact?

Is watching + edit node_modules/ really a feature worth supporting in create-react-app?

A compromise could be, if possible, to detect if watchman is installed. It could be mentioned in the readme as feature "Hey if you install watchman you can live edit and debug stuff in node_modules"

gaearon · 2017-05-29T16:02:08Z

Yea, I think it would be nice if we could detect watchman. When it exists we opt for more aggressive watching (including node_modules). When it doesn't (and system doesn't support watchman) then we don't try it.

I'm also curious why we can't just teach Jest to use chokidar which is what Webpack is using.

thisconnect · 2017-05-29T16:05:37Z

npm test took longer to startup when node_modules are watched (without watchman that is), not measured just noticed.

Timer · 2017-05-29T19:11:33Z

We could use some of your digging in skills in figuring out what's the best place to fix it.

IMO, we need to teach Jest how to use chokidar or, as you said, make watchman a hard dependency.

Yea, I think it would be nice if we could detect watchman. When it exists we opt for more aggressive watching (including node_modules). When it doesn't (and system doesn't support watchman) then we don't try it.

Unfortunately I think this wouldn't be good for cross-machine ejected apps, don't we serialize the Jest config into package.json?

Timer · 2017-05-29T19:13:02Z

I have never installed watchman nor now anything about it.

The fun part is you don't need to know anything about it, all you need to do is brew install watchman.

AFAIK it handles the performance implications of such a large amount of files. I don't even know that much about it, other than it's Facebook's file watcher. 🤷‍♀️

gaearon · 2017-05-29T19:13:06Z

We could do it in the test script itself.

gaearon · 2017-05-29T19:14:21Z

I guess I'm cool with making watchman a required dependency on OS X for watch mode.

Timer · 2017-05-29T19:16:15Z

We could do it in the test script itself.

We could, I'm a little tied up for the rest of today but I can take a look at this tomorrow.

I guess I'm cool with making watchman a required dependency on OS X for watch mode.

Have the weird issues with watchman been sorted out on macOS? I know we had a whole section in our docs dedicated to troubleshooting it.

@ro-savage also seems very willing to help, maybe he could provide a recommendation as well.

gaearon · 2017-05-29T19:30:48Z

Have the weird issues with watchman been sorted out on macOS? I know we had a whole section in our docs dedicated to troubleshooting it.

I think that is the issue: it crashes without watchman. There was another issue about it hanging with watchman but I think (am I wrong? not sure) it was sorted with one of watchman’s updates.

thisconnect · 2017-05-29T19:35:47Z

So it seems you all definitely want to watch node_modules ?

gaearon · 2017-05-29T20:21:22Z

If we have watchman then yes, I think it's fine to watch them.
(Maybe we can use the same watchman detection code that Jest uses.)

And it seems like Node crawler works fine outside of macOS so we only need to not watch it only on macOS without watchman. At this point we might as well require macOS users to install watchman as it seems simpler?

ro-savage · 2017-05-29T21:30:46Z

I think it depends a lot of we can install watchman as part of the normal npm install. If we can, then its not really much of an issue. If the user needs to brew install it feels like overkill when we can just ignore node_modules.

Long term, the issue itself exists in the way node watches files and a fix needs to happen in node. I know in nodejs/node-v0.x-archive/issues/5463 node blamed OSX but seeing as watchman works fine on OSX surely node could work fine as well?

gaearon · 2017-05-29T22:52:14Z

Can you remind me why we can't switch Jest Node crawler to use Chokidar (which works fine on macOS)?

Timer · 2017-05-29T22:59:26Z

Can you remind me why we can't switch Jest Node crawler to use Chokidar (which works fine on macOS)?

In terms of history, I believe chokidar was very unstable/buggy when Facebook needed a watching solution, so Watchman was born.
Maybe we can progress paulmillr/chokidar#486 along.

Timer · 2017-05-29T23:05:45Z

See facebook/react-native#628 and facebook/react-native#5 (comment) for some historical context -- featuring a conversation between @gaearon and @amasad. 😅

I'm not sure if there's a real reason to not switch now.

gaearon · 2017-05-30T00:16:28Z

Watchman still makes sense as first solution. But we should fall back to fsevents on macOS (which chokidar uses when possible). However Jest doesn't seem to fallback to fsevents. That's the problem.

gaearon · 2017-05-30T00:23:40Z

Eh, I still don't understand what's going on 😞

We know Webpack uses Chokidar which uses fsevents on macOS. Nevertheless there is no problem with Webpack.

Jest, on the other hand, produces an error message inside of fsevents. However I can't find a single mention of fsevents or even fs.watch in its Node crawler.

Can somebody explain:

Where does Jest use fsevents?
Why doesn't webpack (which also relies on fsevents) fail?

gaearon · 2017-05-30T00:28:35Z

OK, I understand what happens now. I shouldn't look at "crawler", I should look at "watcher". Which comes from sane which uses fs.watch. Which presumably uses fsevents but for some reason Webpack doesn't have this issue? Maybe Node's version of fsevents backing fs.watch is too old? Could explicitly using fsevents@latest inside sane for compatible systems fix the issue? cc @amasad

amasad · 2017-05-30T00:43:08Z

@gaearon fsevents means two things here it's the OS X native filesystem events and the node module. Watchman, Node, and the fsevents npm module use FSEvents for watching.

I thought this was fixed in recent node releases but maybe it's not: node would overload FSEvents by starting much more watches than it needs. Watchman solves this by multiplexing on the same watch channel. I'm not sure how chokidar solves it.

IMO making watchman a hard dependency is better than taking on fsevents as a dependency. IIRC watchman is now installable via npm right? But also open to suggestions on how to make sane better.

amasad · 2017-05-30T03:22:52Z

Started an FSEventsWatcher branch on sane. Still needs some work, I'll try to finish it soon but someone else want's to jump in feel free to amasad/sane#97

gaearon · 2017-06-27T16:16:22Z

We'll go with @amasad's approach instead. It will make it to Jest within a few weeks.

facebook-github-bot added the CLA Signed label May 28, 2017

ro-savage mentioned this pull request May 28, 2017

react-scripts test is watching files in node_modules #2393

Closed

thisconnect reviewed May 28, 2017

View reviewed changes

ro-savage force-pushed the fix-tests-watching-node-modules branch from 1983cda to 53e4cd3 Compare May 28, 2017 08:56

thisconnect reviewed May 28, 2017

View reviewed changes

ro-savage force-pushed the fix-tests-watching-node-modules branch from 53e4cd3 to 83543a6 Compare May 28, 2017 09:25

ro-savage changed the title ~~Revert to modulePathIgnorePatterns to fix issue with watchers crashin…~~ Fix Jest crashing on projects with large node_modules folders May 28, 2017

ro-savage force-pushed the fix-tests-watching-node-modules branch from 83543a6 to 14e21b2 Compare May 28, 2017 09:39

gaearon reviewed May 28, 2017

View reviewed changes

ro-savage force-pushed the fix-tests-watching-node-modules branch from 14e21b2 to 09b29e2 Compare May 28, 2017 12:32

Revert to modulePathIgnorePatterns to fix issue with watchers crashin…

5bc5076

…g jest

ro-savage force-pushed the fix-tests-watching-node-modules branch from 09b29e2 to 5bc5076 Compare May 28, 2017 12:51

thisconnect reviewed May 28, 2017

View reviewed changes

gaearon added this to the 1.0.x milestone May 29, 2017

gaearon mentioned this pull request May 30, 2017

Watch mode stopped working on macOS Sierra jestjs/jest#1767

Closed

gaearon closed this Jun 27, 2017

lock bot locked and limited conversation to collaborators Jan 21, 2019

Fix Jest crashing on projects with large node_modules folders #2395

Fix Jest crashing on projects with large node_modules folders #2395

Conversation

ro-savage commented May 28, 2017

thisconnect commented May 28, 2017

thisconnect May 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro-savage commented May 28, 2017 • edited Loading

gaearon commented May 28, 2017

cpojer commented May 28, 2017

cpojer commented May 28, 2017

gaearon commented May 28, 2017

Choose a reason for hiding this comment

ro-savage May 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Timer commented May 29, 2017 • edited Loading

gaearon commented May 29, 2017

gaearon commented May 29, 2017

thisconnect commented May 29, 2017

gaearon commented May 29, 2017

thisconnect commented May 29, 2017 • edited Loading

Timer commented May 29, 2017

Timer commented May 29, 2017

gaearon commented May 29, 2017

gaearon commented May 29, 2017

Timer commented May 29, 2017

gaearon commented May 29, 2017

thisconnect commented May 29, 2017

gaearon commented May 29, 2017 • edited Loading

ro-savage commented May 29, 2017 • edited Loading

gaearon commented May 29, 2017

Timer commented May 29, 2017

Timer commented May 29, 2017

gaearon commented May 30, 2017

gaearon commented May 30, 2017

gaearon commented May 30, 2017 • edited Loading

amasad commented May 30, 2017 • edited Loading

amasad commented May 30, 2017

gaearon commented Jun 27, 2017

thisconnect May 28, 2017 •

edited

Loading

ro-savage commented May 28, 2017 •

edited

Loading

ro-savage May 28, 2017 •

edited

Loading

Timer commented May 29, 2017 •

edited

Loading

thisconnect commented May 29, 2017 •

edited

Loading

gaearon commented May 29, 2017 •

edited

Loading

ro-savage commented May 29, 2017 •

edited

Loading

gaearon commented May 30, 2017 •

edited

Loading

amasad commented May 30, 2017 •

edited

Loading