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

WebpackDevServer: (Only) Ignore node_modules if polling is required #3982

Closed
wants to merge 5 commits into from

Conversation

petetnt
Copy link
Contributor

@petetnt petetnt commented Feb 5, 2018

Fixes the most common case of #2956.

This PR changes WebpackDevServer settings to only ignore node_modules if the resource intensive CHOKIDAR_USEPOLLING environment variable is set. AFAIK using the said environment variable is the only way to end up poll watching, as CRA itself doesn't explicitly set polling related options. (edit: wrong, check below)

The PR also ignores the special ignoredFiles set, as CRA now supports monorepos natively. Instead we ignore all node_modules folders with a /node_modules/ RegExp. Removing the special case also removes the tests from react-dev-utils as they were the only related tests to that package.

Not sure if we should also have an escape hatch (like IGNORE_NODE_MODULES_WATCHING env variable) for cases where native file watching could be too intensive anyway?

Also should we warn the users if polling is enable that the node_modules folder is ignored or is the README notice enough?

@gaearon
Copy link
Contributor

gaearon commented Feb 5, 2018

AFAIK using the said environment variable is the only way to end up poll watching, as CRA itself doesn't explicitly set polling related options.

What if fsevents failed to install? I imagine chokidar would also switch to polling.

@petetnt
Copy link
Contributor Author

petetnt commented Feb 5, 2018

Good point and it is true, missed that Chokidar falls back to the polling in that case on OSX:

https://github.com/paulmillr/chokidar/blob/master/index.js#L89

Not quite sure what the strategy catching that would be. Just checking that fsevents exists and requires neatly like it does in https://github.com/paulmillr/chokidar/blob/d1bac5bfe707c71a9f82b81d4c80de0fae3a8715/lib/fsevents-handler.js#L7-L9?

@gaearon
Copy link
Contributor

gaearon commented Feb 5, 2018

Yes but you’d need to make sure you’re trying to resolve it from the same folder. So you might need to recursively resolve the package chain. Sounds kinda complicated.

@gaearon
Copy link
Contributor

gaearon commented Feb 5, 2018

Also what happens on windows? Does it need polling?

@petetnt
Copy link
Contributor Author

petetnt commented Feb 5, 2018

On Windows Chokidar docs state that

On other platforms, the fs.watch-based implementation is the default, which avoids polling and keeps CPU usage down. Be advised that chokidar will initiate watchers recursively for everything within scope of the paths that have been specified, so be judicious about not wasting system resources by watching much more than needed.

So Windows shouldn't use polling unless forced (which is also the impression I got from a quick dive to fs.watch internals.

The resolving thing does sound kinda complicated, let me try something on that.

Thanks for the comments @gaearon 👍

@petetnt petetnt changed the title WebpackDevServer: (Only) Ignore node_modules if CHOKIDAR_USEPOLLING is truthy WebpackDevServer: (Only) Ignore node_modules if polling is required Feb 6, 2018
@petetnt
Copy link
Contributor Author

petetnt commented Feb 6, 2018

@gaearon I did a test of diving into the the cache and confirmed that something like the following does work, tested by first having fsevents installed and then not having fsevents installed.

const watchOptions = {};

// While Create React App does not use polling for watching files by default,
// polling can occur if CHOKIDAR_USEPOLLING environment variable is
// set to a truthy value. Excessive polling can cause CPU overloads
// on some systems (see https://github.com/facebookincubator/create-react-app/issues/293),
// which is why we ignore node_modules if polling is enforced.
let shouldIgnoreNodeModules = false;

const envForceUsePolling = process.env.CHOKIDAR_USEPOLLING;

if (envForceUsePolling) {
  const usePollingLower = envForceUsePolling.toLowerCase();

  // Chokidar rules https://github.com/paulmillr/chokidar/blob/master/index.js#L99
  if (
    usePollingLower === 'true' ||
    usePollingLower === '1' ||
    !!usePollingLower
  ) {
    shouldIgnoreNodeModules = true;
  }
}

if (process.platform === "darwin") {
  // check if fsevents is loaded
  const cacheKeys = Object.keys(require.cache);
  const cachedFsEvents = require.cache[cacheKeys.find(i => i.includes("/fsevents/fsevents.js"))];

  if (cachedFsEvents && cachedFsEvents.loaded) {
    // fsEvents is loaded, check if its WebpackDevServers
    let parentIsWebpackDevServer = false;
    let parent = cachedFsEvents.parent;

    while (parent && !parentIsWebpackDevServer) {
      if (parent.id.includes("webpack-dev-server")) {
        parentIsWebpackDevServer = true;
      }

      parent = parent.parent;
    }

    if (!parentIsWebpackDevServer) {
      shouldIgnoreNodeModules = true;
    }
  } else {
    shouldIgnoreNodeModules = true;
  }
}

if (shouldIgnoreNodeModules) {
  watchOptions.ignored = /node_modules/;
}

Not sure if that just cachedFsEvents && cachedFsEvents.loaded would be enough, but the parent check works too. Does that sound crazy? Caveats include possibly (but very unlikely) the filenames changing or something deleting it from the cache in between WebpackDevServer being required and the config being initialized (not very likely either I guess). Edit: running into a circular ref in the parent loop might happen too.

@gaearon
Copy link
Contributor

gaearon commented Feb 6, 2018

I don't mind something like this as long as it's hidden in react-dev-utils and doesn't negatively impact startup time. (That Object.keys is worth measuring)

@petetnt
Copy link
Contributor Author

petetnt commented Feb 6, 2018

Cool 👌 Did some measurements on my machine and rolling the whole check takes ~500000ns on my machine so it shouldn't be a worry (we can double check on that later). I'll move the check to react-dev-utils and commit the thing in.

@petetnt
Copy link
Contributor Author

petetnt commented Feb 12, 2018

Not sure why the tests flaked 🤔

@viankakrisna
Copy link
Contributor

viankakrisna commented Feb 12, 2018

@petetnt you need to add the shouldIgnoreNodeModules.js to the files property in react-dev-utils's package.json https://github.com/petetnt/create-react-app/blob/0c4bbc9355bc3e733316351813b644477c2f13f4/packages/react-dev-utils/package.json#L13-L37

@petetnt
Copy link
Contributor Author

petetnt commented Feb 12, 2018

Ah, of course. Thanks @viankakrisna

let parent = cachedFsEvents.parent;

while (parent && !parentIsWebpackDevServer) {
if (parent.id.includes('webpack-dev-server')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not super happy about this in particular. In the future we'll migrate off WDS. Do we need to hardcode this at all?
Can you explain more about why this check is necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like mentioned somewhere about, the check is most likely not necessary, but anyway: consider case where ModuleX and WebpackDevServer both load Chokidar, and due to something ModuleX properly loads fsevents but WDS fails to do so -> WDS polls eventhough it didn't successfully load the module.

That said,

  • I don't know if that's even possible 🤔
  • Nothing else in the CRA stack currently depends on Chokidar, so the check is currently rather moot anyway.

I am totally okay with removing it if you are 🆗

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it WDS require-ing fsevents, or webpack? I would assume that webpack should be somewhere on the stack because WDS doesn't (afaik) do any watching manually, it just tells webpack to watch.

I'd be fine to change this line if it referenced just webpack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sadly its WDS that requires chokidar to which the watchOptions are passed in WDS's own implementation of something similar to watchpack which eventually requires fsevents.

@Timer Timer closed this Sep 26, 2018
@Timer Timer reopened this Sep 26, 2018
@gaearon gaearon closed this Nov 1, 2018
@lock lock bot locked and limited conversation to collaborators Jan 18, 2019
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.

5 participants