[core] Enable @typescript-eslint/consistent-type-imports rule

[JCQuintas]

I believe this would be helpful in some instances where the "cyclic import" triggers, but we are only importing types.

The no-cycle documentation mentions

This rule ignores type-only imports in Flow and TypeScript syntax (import type and import typeof), which have no runtime effect.

You can also read more about other benefits

Scope

Currently only added to the charts packages, though we can enable it for the repo if everyone agrees. (a LOT of file changes)

[mui-bot]

Deploy preview: https://deploy-preview-13562--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against ebbcbfd

[cherniavskii]

This makes sense to me!
We can enable this rule for other packages in subsequent PRs 👍🏻

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[LukasTy]

Could you clarify what problem this change should be solving? 🤔
Personally, I'm not a fan of tailoring (convoluting) code in an effort to avoid problems that should be taken care by the transpiler.

Let's take this line as an example.
It already does not end up in the built code: 🤷
https://unpkg.com/@mui/[email protected]/BarChart/BarElement.js
https://unpkg.com/@mui/[email protected]/modern/BarChart/BarElement.js
https://unpkg.com/@mui/[email protected]/esm/BarChart/BarElement.js

[JCQuintas]

I've written a short summary in the description.

The main issue here is that the eslint rule no-cycle will complain about

// first-file.ts
import {Two} from './second-file.ts' // cycle error

export someFunc(value: Two) {}

// second-file.ts
import {someFunc} from './first-file.ts' // cycle error

type Two = { foo: number }

someFunc({foo: 1})

But it will be ok if we import it as types

// first-file.ts
import type {Two} from './second-file.ts'. // ok

export someFunc(value: Two) {}

// second-file.ts
import {someFunc} from './first-file.ts' // ok

type Two = { foo: number }

someFunc({foo: 1})

I've had to manually change some imports to type imports in order to make the cycle issue go away.

Alternative

An alternative option would be to remove the no-cycle rule, which is generally not necessary in JS, but I don't know the reason it is there.

[flaviendelangle]

Another alternative would be to only add type when we do have a cycle import issue and not change the linting config.

This is an issue I encounter from time to time, but it's not super hard to avoid it.

• I try to always add import type when the imports is inside a models folder and targeting a file that is outside of it (because the files in the models are widely imported so when they themselves import something it's often where I have issues).
• I import from the exact files rather than index when it's a private interface

I feel like enforcing import type is a little over-reacting given the scale of the problem (as I encountered it at least). If the charts codebase is a lot more prone to this error, I would be curious to check why this is (maybe the folder structure is not optimal).

But if the majority thinks that it's an improvement, I will clearly not be a blocker.

Do you know if the IDE is able to use import type {} when importing using autocompletion?
Otherwise, I feel like I'll spend more time fixing the imports every time I use autocompletion than fixing a cycle import once every few weeks.

[JCQuintas]

From my point of view, the less things I have to worry about the better, this is something that happens often for me, at least in the way I program.

On your question, this is an auto-fixable eslint rule, so if you have the IDE auto-fix your issues you wouldn't see a problem there.

If we want to enable the IDE to automatically import as type, we would need to also setup the tsconfig rule for this, which I thought would be too aggressive 😅

[LukasTy]

I'm on the same "boat" as Flavien on this one.
At least on Pickers codebase, this problem seems to pop up maybe once or twice a month and sometimes is avoidable only by import type {}, while in other cases we might only need more specific relative imports. 🙈
Maybe you are using the shallowest possible relative imports most of the time? 🤔

I'm also not blocking this change, but I'm not sure we need it on Pickers packages. 🤷

[flaviendelangle]

In the pickers and the tree view, we always put the props in a MyComponent.types.ts file, that's probably the reason why we rarely have those circle deps.

Most of the circle deps I see comes from utils types that ends up being too close from a component instead of being in an helpers file (in the models folders).

IMHO, the main risk with this rule is that is do not push as toward a good file architecture.
Most of the time, when I have a circle dep, it's because my types are not in a good place. Sometime sure I end up with 2 types that are in 2 helper files and each file needs something from the other, but it's rare.

[JCQuintas]

One of my goals was to not propose a whole new project structure 😅

We could definitely solve this with more strict folder architecture, but I'm not in favour of creating rules that aren't/can't be enforced automatically on CI.

The reason for me adding this rule is because there is no need at all for us to worry about cyclic types, as they don't really create side-effects in code, which is the main issue a cyclic import could cause in javascript.

If we have cyclic code, then yes, we are "required" to change the folder structure to comply with the rule.

A small benefit with this rule is that it would make the linter more efficient (although only slightly), because the rules that worry about these, nominally the no-cycle, can just skip checking these imports.

[JCQuintas]

Without rule/changes (66% of time)

Rule                                 | Time (ms) | Relative
:------------------------------------|----------:|--------:
import/no-cycle                      | 33780.192 |    66.1%
@typescript-eslint/naming-convention |  1792.950 |     3.5%
react-compiler/react-compiler        |  1259.460 |     2.5%
import/no-unresolved                 |  1195.718 |     2.3%
@typescript-eslint/no-redeclare      |   902.521 |     1.8%
@next/next/no-html-link-for-pages    |   789.038 |     1.5%
react/no-unstable-nested-components  |   775.020 |     1.5%
react/no-direct-mutation-state       |   640.183 |     1.3%
import/extensions                    |   540.995 |     1.1%
react/default-props-match-prop-types |   522.282 |     1.0%

With rule/changes on all releasable packages (60% of time)

Rule                                 | Time (ms) | Relative
:------------------------------------|----------:|--------:
import/no-cycle                      | 25968.342 |    60.0%
@typescript-eslint/naming-convention |  1736.135 |     4.0%
react-compiler/react-compiler        |  1434.730 |     3.3%
import/no-unresolved                 |  1203.696 |     2.8%
@typescript-eslint/no-redeclare      |   907.395 |     2.1%
@next/next/no-html-link-for-pages    |   793.412 |     1.8%
react/no-unstable-nested-components  |   776.741 |     1.8%
react/no-direct-mutation-state       |   627.720 |     1.4%
import/extensions                    |   523.839 |     1.2%
react/default-props-match-prop-types |   514.406 |     1.2%

[LukasTy]

The performance benefit seems like a nice win. 👍
However, forcing a type prefix for type imports might hide such inefficient file structures as here: mui/material-ui@781eda5 🙈
Only after fixing the ESLint no-cycle rule to run on the said package do you notice the problems. 🙈

[MBilalShafi]

I also faced issues with cyclic imports but very rarely, mostly when working on large features and the file structure is not well distributed. I feel it could be slightly helpful especially if it brings performance benefits too. 👍

Sidenote: I like this way of naming the local types files: MyComponent.types.ts. It might be a nice convention we could adapt project-wide to avoid cyclic import errors and organize typescript chunks of the code to nice/predefined spots.
It may also help avoid the issues like the one mentioned by Lukas in the last comment. 🤷‍♂️

[github-actions]

This pull request has conflicts, please resolve those before we can evaluate the pull request.

[JCQuintas]

Thanks for the input everyone, I'll be closing this in favour of #13633