import/no-cycle does not detect some cycles with side-effect imports

[guillaumebrunerie]

Here is the setup (https://codesandbox.io/p/devbox/my57j6):

// a.js
import "./b";
export const f = () => globalThis.x;

// b.js
import "./c";

// c.js
import { f } from "./a";

globalThis.x = f();

This seems like a cycle to me (a.js expects globalThis.x to be set given that it imports b.js which imports c.js which sets it, but c.js uses an import from a.js in order to set globalThis.x).
Yet this is not detected as a cycle by eslint-plugin-import. Note that changing a.js to import ./c instead of ./b is detected as a cycle, so it seems to be the indirection introduced by b.js which confuses no-cycle.

[ljharb]

It’s not a cycle; it’s just a global variable (which you never want to be using).

[guillaumebrunerie]

Well, a imports b which imports c which imports a. Isn’t that a cycle? The global variable is just there to show that it creates problems.
The actual problem I’m having is Webpack complaining about "cannot access variable before initialization", which in my experience is usually due to cycles, but import/no-cycle doesn’t report any error.
I now managed to fix my error by shuffling things around, but it still seems to me that import/no-cycle should report it.

[ljharb]

Oh, fair point. Yes, that is a cycle. What's your config set to, and what version of the plugin are you using?

[guillaumebrunerie]

Oh, fair point. Yes, that is a cycle. What's your config set to, and what version of the plugin are you using?

I’m using the latest version (2.31.0) with the latest version of ESLint (9.18.0) and the following config (taken mostly from the docs of eslint-plugin-import):

import importPlugin from "eslint-plugin-import";
import js from "@eslint/js";

export default [
  js.configs.recommended,
  importPlugin.flatConfigs.recommended,
  {
    files: ["**/*.{js,mjs,cjs}"],
    languageOptions: {
      ecmaVersion: "latest",
      sourceType: "module",
    },
    rules: {
      "import/no-cycle": "warn",
      "import/default": "off",
    },
  },
  {
    ignores: ["dist"],
  },
];

[ljharb]

ok, then it should be using scc. cc @soryy708

[soryy708]

Ok we're talking about a false-negative.
Lets check if the SCC optimization is responsible for it.
Try adding disableScc: true. Does no-cycle report it as a cycle then? or still no?

[soryy708]

I have looked in the attached codesandbox.
Why does it have both an .eslintrc.json and an .eslint.config.js?
Wouldn't that confuse ESLint?

[guillaumebrunerie]

Ok we're talking about a false-negative. Lets check if the SCC optimization is responsible for it. Try adding disableScc: true. Does no-cycle report it as a cycle then? or still no?

It makes no difference to add disableScc: true, the cycle still isn’t detected.

[guillaumebrunerie]

I have looked in the attached codesandbox. Why does it have both an .eslintrc.json and an .eslint.config.js? Wouldn't that confuse ESLint?

Hmm, not sure, I simply took some default codesandbox and added ESLint and eslint-plugin-import.
In my actual project, I do have both a eslint.config.js and a .eslintrc, but this is because the .eslintrc is needed for ignorePatterns to work properly with import/no-unused-modules.
Also, changing a.js to directly import ./c instead of ./b makes ESLint detect the cycle, so it does seem like ESLint is working properly.

[soryy708]

Cool.

In a.js:

import "./b.mjs";

is treated as a type-import,
we exit early in:

• eslint-plugin-import/src/rules/no-cycle.js

  Line 98 in 341178d

  return; // ignore type imports

[ljharb]

Why would that be a type import?

[soryy708]

I don't know. Probably shouldn't?

Perhaps because b.js has no exports?

[ljharb]

It definitely shouldn’t - if something with no exports is imported, it’s for side effects, and that means it’s critical and must always be considered.

[guillaumebrunerie]

we exit early in:

eslint-plugin-import/src/rules/no-cycle.js

Line 98 in 341178d

return; // ignore type imports

Looks like this also ignores imports of the form import { type Foo } from "foo", but it is worth noting that with the verbatimModuleSyntax TypeScript option, this form of import is used to import both a type and its side effects, so it should not be ignored.
Now I don’t know if it’s possible/desirable to have eslint-plugin-import behave differently depending on TypeScript options, but maybe only type imports of the form import type { Foo } from "foo" should be ignored by default? Or conversely there could be a verbatimModuleSyntax option for the import/no-cycle rule.

[ljharb]

We do look at tsconfig for a flag already; I'm sure we could look up verbatimModuleSyntax.

[soryy708]

So we have at least 3 false-negatives here:

1. JS module style import '...';
2. TS import type {...} from '...'; with verbatimModuleSyntax: true
3. TS import { type ... } from '...'; with verbatimModuleSyntax: true

Would you like to contribute test cases?

[soryy708]

On another topic,

On one hand fixing a false-negative would technically be a bugfix (so perhaps "patch" in semver?),
but on the the other failing on what used to be fine before would technically be a breaking change (so perhaps "major" in semver?)

How will we want to handle these edge cases?

[ljharb]

semver in eslint plugins is a bit tricky; producing more warnings can be a patch because it was a bug to miss them.

[soryy708]

apropos verbatimModuleSyntax:

• [import/consistent-type-specifier-style]: add exceptions #2872