Add warning for circular dependencies

[whmountains]

One of the things I love about create-react-app is how it makes it hard to break things. Things like the CaseSensitivePathPlugin and the ESLint error overlays catch mistakes early so you don't have to manually debug them.

I suggest adding a similar warning for circular dependencies. It's a common mistake, especially for beginners, and the symptoms are non-obvious. I'm not familiar with the Webpack API but at least in theory it should be easy to notice cycles by inspecting the dependency tree.

I think it would be overkill to fail the build for an error like this, because it is possible to write code that handles this correctly. Ideally you could just include the warning in all the usual places where ESLint warnings are displayed.

[Timer]

Unfortunately, I think it's really hard to statically analyze all of the possible cases and I'm not sure of the performance implications, but it's something we can definitely put on the road map and have some discussion about it.

Could you provide some cases where you've seen this go wrong?

[whmountains]

Could you provide some cases where you've seen this go wrong?

Yes I can. I make that mistake all the time :) When importing a function, I only notice the problem if I happen to call it on startup. When importing a value, I've had cases where my code silently accepts it but then does unexpected things.

For example, yesterday I made the mistake of creating a loop involving the file containing my Redux action type constants. It probably wasn't the best code style, but in any case it resulted in all the action types silently becoming undefined which then made them all match the {} empty action that combineReducers dispatches on init.

This in turn messed up my state because the reducer wasn't receiving the parameters it expected. There was no error, only my state was invalid and I couldn't figure out why because no action had been triggered.

Other times I've realized there must be a cycle, but I couldn't find it until I created a copy of my app, ejected from create-react-app, then ran webpack --config config/webpack.config.dev.js --json --profile > stats.json and opened that in webpack's bundle inspector. From there I could explore the dependency visualization and it became obvious.

[whmountains]

Here's a quick and dirty proof-of-concept: https://github.com/whmountains/create-react-app-circular-deps-poc

Just download it and run yarn start to see the cycle-detection in action.

[whmountains]

On that proof-of-concept app, checking for cycles takes about 3ms. I also tested the same code in a large webapp that I'm developing with ~100 source files and it took ~28ms.

The tradeoff that I had to make was to not scan node_modules, but I think that's a reasonable assumption.

[gaearon]

Hmm, aren’t circular dependencies a supported (even if discouraged) feature of both CommonJS and ES6 modules?

[whmountains]

Yes they are, as far as I know, although I've never come across a case where it wasn't a bug. Maybe we could create some kind of flag to disable the warning for the rare case where it's intentional.

I'm hoping to help beginners out, but I will understand completely if the maintainers decide that it's beyond the scope of create-react-app.

[c10b10]

I've been using https://github.com/aackerman/circular-dependency-plugin to deal with circular dependencies. A flag that activates that Webpack plugin would be very useful. Or is it possible to add webpack plugins ourselves?

[Timer]

I don't believe we're going to go forward with this because a circular dependency doesn't imply a bug.
I would only feel comfortable adding this if it worked in recursive cases (I would imagine this doesn't work?), e.g.:

// moduleA.js
import { bar } from './moduleB'

function foo() {
  bar()
}

// moduleB.js
import { foo } from './moduleA'

function bar() {
  foo()
}

Sorry!

[gaearon]

There are valid use cases for circular deps in my experience. We’ll err on the side of allowing what’s valid per spec.

[whmountains]

I totally understand where you're coming from. Thanks for giving the idea a fair shake, anyway.

[gaearon]

Thanks for the suggestion!

[RyanAtBarefoot]

@c10b10 I'm trying to get circular-dependency-plugin working with my create-react-app based setup and I'm having a hard time getting it to work. Was there a trick to it? All I did was eject and then update webpack.config.dev as shown below. Am I missing something as it's not working which I know for sure because I just fixed and circular dependency error.

...
    plugins: [
      // Prevents users from importing files from outside of src/ (or node_modules/).
      // This often causes confusion because we only process files within src/ with babel.
      // To fix this, we prevent you from importing files out of src/ -- if you'd like to,
      // please link the files into your node_modules/ and let module-resolution kick in.
      // Make sure your source files are compiled, as they will not be processed in any way.
      new ModuleScopePlugin(paths.appSrc, [paths.appPackageJson]),
      new CircularDependencyPlugin({
        // exclude detection of files based on a RegExp
        exclude: /node_modules/,
        // add errors to webpack instead of warnings
        failOnError: true
      }),
    ],
  },
  ...