Allow cyclic dependencies for import/no-cycle iff mixing static/dynamic imports

[D-Pow]

Request

Add a new option for import/no-cycle to allow/warn/error about cyclic dependencies if, and only if, the cycle involves dynamic imports.

Problem Statement

There exists a use case for web apps where some modules are stored in a form of global state and then other modules import that state to update it. For example, an (overly simplistic/half-pseudo code) purpose for this might look something like:

// state.js
import Comp1 from './Comp1';
import Comp2 from './Comp2';
import Comp3 from './Comp3';

const routeModuleMap = {
    '/path1': Comp1,
    '/path2': Comp2,
    '/path3': Comp3,
};

const routeOrder = [
    '/path1',
    '/path2',
    '/path3',
];

let state = 0;

export function getActiveModule() {
    return routeModuleMap[location.pathname];
}

export function setActiveModule(dx = 1) {
    if (dx === 0) {
        return;
    }

    if (dx < 0) {
        state = Math.max(0, state + dx);
    }

    if (dx > 0) {
        state = Math.min(routeOrder.length - 1, state + dx);
    }

    history.pushState(null, null, routeOrder[state]);
}

export default const Context = React.createContext({ getActiveModule, setActiveModule });


// parent.jsx
import Context, { getActiveModule, setActiveModule } from './state';

export default function App() {
    const Component = getActiveModule();

    return (
        <Context.Provider value={{ getActiveModule, setActiveModule }}>
            <Component />
        </Context.Provider>
    );
}


// Comp1.jsx
import Context from './state';

export default Comp1() {
    const { setActiveModule } = useContext(Context);

    return (
        <button onClick={() => setActiveModule()}>
            Go to next page
        </button>
    );
}

Obviously this has a circular dependency, so when bundled, it might result in CompN is not defined errors.
However, that can easily be resolved by mixing static and dynamic imports, which return promises instead of the code itself. This means that all files are parsed/run before any promises resolve, meaning that we never have X is not defined errors.

i.e.

// old
import Comp1 from './Comp1';
import Comp2 from './Comp2';
import Comp3 from './Comp3';

const routeModuleMap = {
    '/path1': Comp1,
    '/path2': Comp2,
    '/path3': Comp3,
};


// new
const routeModuleMap = {
    '/path1': React.lazy(() => import('./Comp1')),
    '/path2': React.lazy(() => import('./Comp2')),
    '/path3': React.lazy(() => import('./Comp3')),
};

While this is a very simple and contrived example, there are cases where some sort of global state links to modules, and each module needs to modify that global state. In these cases, mixing static with dynamic imports solves the problem of code not being defined because your bundler of choice converts A --> B --> A to A --> B --> Promise.resolve(A).

Of course, having circular dependencies in your code is an anti-pattern and reflects brittle code that should still be refactored. This is a great plugin/rule to do exactly that: fix old code and guide new code standards.
But for gigantic repositories, those can't all be fixed at once, especially when the code is in more complex global state. For those specific situations, it'd be really helpful to be able to toggle dynamic-import-cycles using dynamic imports from allow --> warn --> error gradually as they are addressed.
This would mean errors are still shown for static import cycles, protecting new code while old code is refactored.

[ljharb]

(for future reference, emoji-reacting to your own comment is pretty strange)

I'm not sure exactly what you're asking. You want an option so cycles are not tracked through dynamic import entry points?

In general, the transition path for all rules for a large legacy codebase is "eslint overrides", not "add config options to rules".

[D-Pow]

I did look into overrides, but those are usually for specific files, not specific uses. And we generally want the import/no-cycle rule to error at all times, it's just that cyclic dependencies when using dynamic imports technically doesn't make the code nearly as fragile as when the cycles use static imports. (Again, cycles should be removed, but at least dynamic imports makes them not crash the whole app.)

The request for a new option to the rule was for that case: We want all static import cycles to error and all dynamic import cycles to warn.

[ljharb]

You can use an inline override comment for specific uses, but you're right that gets tedious if there's many of them.

The reason for the rule isn't just to avoid crashing the app, it's to avoid the actual maintenance nightmare of having conceptually cyclic dependencies - the rule just can't catch all forms of them.

It seems like it would be feasible to have an option as you request; I'm just not sure yet it would be a good idea.

[D-Pow]

Oh, yeah you're right there are inline overrides (sorry I'm not communicating all my thoughts until I realize something was unclear). The issue with inline overrides is they don't work where the dynamic imports lie, they only work over the static imports.

i.e.

/* Comp1.jsx */
// Shows error
import Context from './state';



/* Comp2.jsx */
// Shows error
import Context from './state';



/* state.js */
// No error here, though this is where I'd like the error to be.
// That way, I can do the following:

// TODO Fix circular dependencies in complex state logic
// eslint-disable import/no-cycle
const routeModuleMap = {
    '/path1': React.lazy(() => import('./Comp1')),
    '/path2': React.lazy(() => import('./Comp2')),
    '/path3': React.lazy(() => import('./Comp3')),
};

Still, regarding what you said, I 100% agree it's a nightmare to deal with them as weird bugs pop up when least expected. It's just that, in our case, the reason I even discovered this plugin was to use this rule because a major app crash appeared from nowhere due to cyclic deps, so we want to prevent them in the future.

I removed all the static cycles, but the cycles in global state will take more time/effort/care to remove which I can't do alone. So I replaced them with dynamic imports to ensure no bugs/crashes appear without me accidentally breaking something in the refactor. With the desire to prevent them from occurring again, I'd like to commit this rule in our repo, but I can't until the original team can do a more in-depth refactor of a lot of code. Which was why I asked if there could be a new option, to prevent new cycles while we continue to work on these final ones.

That being said, I understand your hesitation since even dynamic import cycles should still be removed. I request it mostly just to help with migrations from bad to good code.

[arpowers]

@ljharb I just ran into this issue also.

Due to the latest ESLint updates, dynamic imports break this rule (they were ignored before)

Even dynamic imports in callbacks: () => import('./module')

This rule is mostly helpful as it applies to hoisted imports (regular es6 imports)...

but I agree w @D-Pow that we need to provide an option to disable cycle checking with dynamic imports; or at least with dynamic imports that exist in callbacks.

[ljharb]

@arpowers is your use case the same (where you intentionally used dynamic imports to hack around this plugin's incorrect behavior of ignoring them), or do you have a different use case where these cycles aren't actually a problem?

[arpowers]

Dynamic imports are resolved when/where the developer needs them, so inherently they are something different entirely from static imports (as they relate to cyclical dependency)

In this case, we use a mapped helper object for routes. We then call these routes from components: e.g. getRoute('myRoute').. the trouble is that components are both imported in the object, but also might make use of the getRoute function.

It works fine, as the dynamic imports happen in a controlled fashion (when they're needed). However, it throws a lint error as of the latest update.

[ljharb]

Sure, but it's still a cycle - you're still importing something from a file that itself depends on that file. Circular deps "work fine" with static imports too - the issue isn't that it doesn't work, it's that it's an inherently unmaintainable and poorly designed application architecture.

[arpowers]

it's an inherently unmaintainable and poorly designed application architecture.

That's overly generalized. Break this down with first-principles.

Q: What's the point of preventing cyclic dependency?
A: To prevent load order issues which are often difficult to identify and fix

As dynamic imports are loaded in a controlled fashion, they should NOT be grouped in with static imports. Simple as that.

Additionally, TypeScript transpiler doesn't always accommodate load order with static imports to "spec" so they don't always "work fine" which is my primary motivation to use this rule.

[D-Pow]

@ljharb you intentionally used dynamic imports to hack around this plugin's incorrect behavior of ignoring them

I think you misunderstood me, but I see where you're coming from. So, to clarify:

• The dynamic imports weren't meant to hack around the plugin, they were added by me solely to prevent the app from breaking from static import cycles (more on that below).
• The plugin's behavior isn't necessarily incorrect, it's actually good to point out cyclic dependencies regardless of static vs dynamic imports!

@ljharb Circular deps "work fine" with static imports too

Actually, they don't; at least, not when using a bundler. Assuming you've seen the output for e.g. Webpack builds, you'd see something like

// index.js
var Comp1 = __webpack_require__('./Comp1')(internalLogic);
return Comp1(props);

// Comp1.js
var State = __webpack_require__('./state')(internalLogic);
return function (props) { return React.createComponent(...) };

// state.js
var routeModuleMap = {
    /**
     * ERROR HERE: Cannot call `internalLogic` of `undefined`
     * where `undefined` should be `function Comp1`
     */
    '/path1': __webpack_require__('./Comp1')(internalLogic),
    // ...
};
// ...
return { stateObjectsUsing: routeModuleMap };

However, if '/path1': require('./Comp1')(...) were replaced with '/path1': new Promise(res => res(require('./Comp1')(...))), then suddenly we wouldn't get that error because it's put in the queue at the end of the JS event loop. In other words:

1. All three of those files are parsed completely.
2. The page loads and does its thing.
3. Now that page loading is done, let's try to resolve these promises.
4. Oh cool, the promises resolved! Let's return the component they contained.

So @arpowers is right that "inherently they are something different entirely from static imports." Not trying to say they're good or bad, but they are technically different.

it's an inherently unmaintainable and poorly designed application architecture.

Yes, it is! That's why I went through and fixed what I could (the static imports). I'm not the original writer of this code, so I can't change the global context state alone. BUT I wanted to prevent anyone else from adding in cycles -- static or dynamic -- to help try to fix the poorly designed architecture, which is why I wanted to at least commit the import/no-cycle rule. I ended up just commenting eslint-disable for the static imports even though the actual issue was not in Comp1, it was in State (very annoying b/c now the eslint-disable comments have no context and don't show the true TODO, which is State).

TL;DR

• You're not wrong that cycles shouldn't exist, regardless of static or dynamic.
• However, static cycles will break the app while dynamic cycles are simply just a code smell/anti-pattern/bad design.
• IMO, the desire for this option is solely to be able to commit the import/no-cycle rule into the repo while still being able to do incremental improvements on that bad code until there are no more cycles.
• (Side note) I guess some people might intentionally want to do this like @arpowers mentioned, so the option could help them, too.

[arpowers]

@D-Pow regarding any sort of circular import, there are some patterns where the benefits outweigh the downsides.

Outside of that I agree w your analysis; I've experienced the same issues.

In practice, circular deps should be avoided unless you know what you're doing. That's for sure.

[D-Pow]

Right, that's what I meant by saying

Not trying to say they're good or bad, but they are technically different.

But yeah, the point was mostly that it's good to point out the dynamic imports; however, it should be optional whether dynamic imports in particular should be errors, warnings, or ignored.

Static cycles should always be errors IMO. Granted, technically it'd be nice to be able to configure each one separately from each other, but, at the very least, dynamic imports should be configurable.

[ljharb]

I'm saying there are no patterns where circular dependencies' benefits outweigh the downsides - that's the purpose of this rule, to prevent you from EVER, under ANY CIRCUMSTANCES, having a circular dependency.

In other words, they are decidedly bad, and this rule exists to prevent them - whether static or dynamic.

Static cycles don't break the app much more than dynamic ones do - both CJS and ESM are designed to that circular dependencies work "fine".

@D-Pow I agree that your incremental use case is worth considering.

[D-Pow]

Don't get me wrong, I do believe that the cons outweigh the pros in every situation. But I wasn't going to claim it as an absolute truth because it's virtually impossible to prove it; e.g. Idk what @arpowers design is (not trying to ask what it is, mind you, just as an example), but maybe there exists some case in some world where they're acceptable. But let's not beat this dead horse too much longer, I think we've covered the conceptual portion enough.

Regardless, I think this entire discussion can be summed up as:

1. Cyclic dependencies are bad.
2. "Bad" is a spectrum -- in high level terms/not related to cyclic deps: breaking the app is the worst and somewhat unclean code is one of the least.
3. People want to use this plugin to help remove cyclic deps.
4. Since dynamic imports are "bad" but not "breaking-the-app bad," an option should be added until the dynamic import cycles can be removed because they exist on different levels of "bad."

Anyway, @ljharb you said you'd consider it, so the ball's in your court. Thanks for your time and discussion. Let's leave this issue open until a decision and/or fix is made.

[arpowers]

@ljharb

You guys realize we are talking about imports within callbacks here?:

() => import('./file')

This rule flags imports within callbacks as circular... As we never call these callbacks within a file, no circular reference is actually made; it still trips this rule.

The use case is a giant mapped utility that enables us to centrally manage our routing. For us, its clearly a positive pattern with no runtime downsides, and with no "actual" circular references at runtime.

[Anber]

Completely agree with @arpowers. We also use dynamic imports in callbacks for referring states in the router. It doesn't cause actual circular reference but it allows us to use strictly typed links and check on compile-time that referred state exists. And I don't see any downsides here.

[ljharb]

All of these are still conceptually circular dependencies. That's the downside, and it remains for every provided use case because cycles are bad architecture, harder to maintain, etc.

I do hear that there are a number of use cases, and that drawing a line between "all imports" and "only static imports" may be useful despite the inherent downsides. I'm still evaluating that.

[GerkinDev]

My 2 (useless) cents on this: this should be configurable IMHO. Even if they are dangerous, they can be used carefully.

And it's even more dangerous to disable this rule altogether because there's currently no way to allow only static + dynamic cycle, leaving us with the risk of static + static madness.

[ljharb]

I'd be willing to consider a PR that adds an option, that is appropriately scarily named and well-documented to discourage its use, that suppresses cycle warnings for dynamic imports.

[GerkinDev]

Well, I'm willing to give it a try! I'm super confused by some parts of the project, and I've never really touched typescript/javascript ast-related code, but hey, there's a first to everything!

[GerkinDev]

Thank you for the merge!