[core-infra] Check dependency cycles inside packages directory only

[michaldudak]

Improve linting time by checking for cyclic dependencies in production code only.

Results from my local machine:

Before:

Rule                                 |  Time (ms) | Relative
:------------------------------------|-----------:|--------:
import/no-cycle                      | 227269.294 |    81.7%
import/extensions                    |  10279.738 |     3.7%
import/no-unresolved                 |   4367.926 |     1.6%
@typescript-eslint/naming-convention |   3440.758 |     1.2%
import/order                         |   2216.073 |     0.8%
import/no-duplicates                 |   1916.750 |     0.7%
import/no-named-as-default           |   1767.365 |     0.6%
import/no-import-module-exports      |   1427.037 |     0.5%
@typescript-eslint/no-redeclare      |   1282.992 |     0.5%
react/default-props-match-prop-types |   1185.290 |     0.4%
Done in 309.20s.

After:

Rule                                 | Time (ms) | Relative
:------------------------------------|----------:|--------:
import/no-cycle                      | 47719.902 |    46.9%
import/extensions                    |  8183.325 |     8.0%
import/no-named-as-default           |  7344.888 |     7.2%
@typescript-eslint/naming-convention |  3636.665 |     3.6%
import/no-unresolved                 |  3424.987 |     3.4%
import/order                         |  2215.172 |     2.2%
import/no-duplicates                 |  1665.064 |     1.6%
import/no-import-module-exports      |  1553.346 |     1.5%
@typescript-eslint/no-redeclare      |  1536.921 |     1.5%
react/default-props-match-prop-types |  1194.546 |     1.2%
Done in 135.13s.

[mui-bot]

Netlify deploy preview

https://deploy-preview-37223--material-ui.netlify.app/

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against a9f7d9c

[oliviertassinari]

@michaldudak Should we update https://github.com/mui/mui-x as well? I imagine their files: ['packages/*/src/**/*{.ts,.tsx,.js}'], would need the same update.

[michaldudak]

Should we update https://github.com/mui/mui-x as well? I imagine their files: ['packages//src/**/{.ts,.tsx,.js}'], would need the same update.

In X, there is no difference in linting time when this option is applied only under packages.

[oliviertassinari]

@michaldudak So you are confirming my proposal? My point is that this PR disable the rule for MUI X, but it seems that we want them to have it enabled. Hence the need to open a PR on their repository.

[michaldudak]

My point is that this PR disable the rule for MUI X

What makes you believe so? I've just linted the MUI X codebase, and it seems to work properly - the import/no-cycle rule is the most expensive one.

Rule                                 | Time (ms) | Relative
:------------------------------------|----------:|--------:
import/no-cycle                      | 52707.177 |    69.3%
import/extensions                    |  2802.054 |     3.7%
import/export                        |  2164.927 |     2.8%
@typescript-eslint/naming-convention |  2141.712 |     2.8%
@typescript-eslint/no-redeclare      |   854.962 |     1.1%
import/no-duplicates                 |   825.792 |     1.1%
import/order                         |   819.804 |     1.1%
import/no-unresolved                 |   809.963 |     1.1%
react/no-unstable-nested-components  |   736.456 |     1.0%
import/no-named-as-default           |   680.766 |     0.9%
Done in 97.21s.

---

I made a mistake when measuring linting time improvements in the X repo before. It seems that with this PR, there's a 20s speedup on this rule:

Results from before this PR:

Rule                                 | Time (ms) | Relative
:------------------------------------|----------:|--------:
import/no-cycle                      | 72112.575 |    77.4%
import/extensions                    |  2863.270 |     3.1%
@typescript-eslint/naming-convention |  1912.485 |     2.1%
import/no-unresolved                 |  1064.251 |     1.1%
import/no-duplicates                 |   979.588 |     1.1%
import/order                         |   911.633 |     1.0%
react/no-unstable-nested-components  |   768.120 |     0.8%
@typescript-eslint/no-redeclare      |   762.009 |     0.8%
react/default-props-match-prop-types |   634.973 |     0.7%
import/no-import-module-exports      |   629.111 |     0.7%
Done in 113.14s.

[oliviertassinari]

@michaldudak In MUI X, import/no-cycle was used to run for the whole codebase. Now it runs for the packages only (since it imports this config). My thought was that maybe we would want to keep the previous behavior there but it's probably not for the best, the tradeoff we have in the main repo is likely the same as we want in X and everwhere else 👍

A quick test on MUI X:

Before this PR

After this PR

This rule is insane 🙃 import-js/eslint-plugin-import#2348

[michaldudak]

@mui/x - it's your call.

[LukasTy]

mui/x - it's your call.

Am I missing something?
I've tried running it without the change, adding the change manually (it increased the runtime even further) and also using the newest material-ui repo version with the change present.
The eslint runtime did not differ for me and remained ~61s on M1 Pro... 🤷

How did you @oliviertassinari reproduce the difference you presented? 🤔

[oliviertassinari]

I think that we can call it a day then, it's not very important, plus it makes us use the same config everywhere.

@LukasTy To reproduce my run, you need to run the latest version of the repo and run yarn eslint:ci. You will get "After this PR". Then you need to add import/no-cycle: 'error' back to get "Before this PR".

[LukasTy]

I think that we can call it a day then, it's not very important, plus it makes us use the same config everywhere.

Yeah, the case is already closed. mui-x repo is already benefitting from this improvement, thanks! 👍

To reproduce my run, you need to run the latest version of the repo and run yarn eslint:ci. You will get "After this PR". Then you need to add import/no-cycle: 'error' back to get "Before this PR".

Sorry, my mistake, I didn't clarify that this change was already included in the current version of material-ui that mui-x references, so, didn't think of reverting this change... 🙈
As mentioned in the previous section reply, it's already applied and also shortens the x repo pipeline runtime. 🚀 💯

Before:

After:

[oliviertassinari]

I have found an unintended regression after these changes (which I don't think this PR is the true root cause). I have created mui/mui-x#10114 so we can keep tabs on it.