import-js / import-js/eslint-plugin-import

[no-cycle] a possible reason of the performance issue

Open
#3,255 2 comments 0 reactions 0 assignees View on GitHub
Dominant language
JavaScript
Stars
5.9k
Forks
1.5k
PR merge metrics
No merged PRs in 30d

Description

I noticed that many issues have reported the performance degradation issue with no-cycle. I wasn't sure which issue to comment my findings, so I decided to open a new one.

Relevant issues:
* https://github.com/import-js/eslint-plugin-import/issues/3148
* https://github.com/import-js/eslint-plugin-import/issues/3047
* https://github.com/import-js/eslint-plugin-import/issues/3060
* https://github.com/import-js/eslint-plugin-import/issues/3113

# Reason

`disableScc: false` always traverse the files in node_modules if:
* `import/extensions` or `import/parsers[parser]` allow the extensions of the files in node_modules (like `.js`, `.jsx`)
* `import/ignore` doesn't contain `node_modules`

`ignoreExternal: true` doesn't work with `disableScc: false`

# Possible Solution

make `ignoreExternal: true` work with `disableScc: false`

# Tests

`maxDepth` is set to `∞`

1. `disableScc: false`, `ignoreExternal: false`, `import/ignore` doesn't contain `node_modules`

```
Rule | Time (ms) | Relative
:---------------------------------------|----------:|--------:
import/no-cycle | 12518.457 | 87.0%
@typescript-eslint/no-unsafe-assignment | 624.835 | 4.3%
prettier/prettier | 476.463 | 3.3%
@typescript-eslint/no-misused-promises | 81.059 | 0.6%
import/no-relative-packages | 80.742 | 0.6%
@typescript-eslint/no-unsafe-return | 74.946 | 0.5%
import/order | 64.003 | 0.4%
import/no-duplicates | 51.688 | 0.4%
import/no-self-import | 41.741 | 0.3%
@typescript-eslint/naming-convention | 37.769 | 0.3%
```

2. `disableScc: false`, `ignoreExternal: true`, `import/ignore` doesn't contain `node_modules`

```
Rule | Time (ms) | Relative
:---------------------------------------|----------:|--------:
import/no-cycle | 12802.937 | 87.4%
@typescript-eslint/no-unsafe-assignment | 619.635 | 4.2%
prettier/prettier | 477.088 | 3.3%
import/no-relative-packages | 79.608 | 0.5%
@typescript-eslint/no-misused-promises | 73.669 | 0.5%
import/order | 60.475 | 0.4%
@typescript-eslint/no-unsafe-return | 51.776 | 0.4%
@typescript-eslint/naming-convention | 47.074 | 0.3%
import/no-duplicates | 46.995 | 0.3%
import/no-self-import | 43.525 | 0.3%
```

3. `disableScc: false`, `ignoreExternal: true`, `import/ignore` contains `node_modules`

```

Rule | Time (ms) | Relative
:---------------------------------------|----------:|--------:
@typescript-eslint/no-unsafe-assignment | 661.048 | 26.5%
import/no-cycle | 563.195 | 22.5%
prettier/prettier | 493.190 | 19.7%
import/no-relative-packages | 81.750 | 3.3%
@typescript-eslint/no-misused-promises | 77.599 | 3.1%
import/order | 62.108 | 2.5%
@typescript-eslint/no-unsafe-return | 51.690 | 2.1%
import/no-duplicates | 50.320 | 2.0%
import/no-self-import | 45.716 | 1.8%
@typescript-eslint/naming-convention | 40.678 | 1.6%
```

4. `disableScc: true`, `ignoreExternal: false`

```
Rule | Time (ms) | Relative
:---------------------------------------|----------:|--------:
import/no-cycle | 35442.592 | 94.9%
@typescript-eslint/no-unsafe-assignment | 601.643 | 1.6%
prettier/prettier | 513.087 | 1.4%
import/no-relative-packages | 81.385 | 0.2%
@typescript-eslint/no-unsafe-return | 76.666 | 0.2%
@typescript-eslint/no-misused-promises | 75.370 | 0.2%
import/order | 60.338 | 0.2%
import/no-duplicates | 57.670 | 0.2%
import/no-self-import | 44.472 | 0.1%
@typescript-eslint/naming-convention | 42.661 | 0.1%
```

5. `disableScc: true`, `ignoreExternal: true`

```
Rule | Time (ms) | Relative
:---------------------------------------|----------:|--------:
@typescript-eslint/no-unsafe-assignment | 667.579 | 26.3%
import/no-cycle | 570.987 | 22.5%
prettier/prettier | 500.888 | 19.8%
@typescript-eslint/no-misused-promises | 83.531 | 3.3%
import/no-relative-packages | 79.151 | 3.1%
import/order | 63.131 | 2.5%
@typescript-eslint/no-unsafe-return | 53.353 | 2.1%
import/no-duplicates | 49.024 | 1.9%
import/no-self-import | 46.231 | 1.8%
@typescript-eslint/naming-convention | 43.361 | 1.7%
```

From above tests, we can see a huge performance improvement from `disableScc: true, ignoreExternal: false` to `disableScc: false, ignoreExternal: false`. However `ignoreExternal` doesn't work with `disableScc: false` thus `disableScc: false` performs worse than `disableScc: true, ignoreExternal: true`

# Reproduce Steps

1. `git clone https://github.com/yifanwww/easy-projs.git`
2. `git checkout chore/eslint-no-cycle`
3. `pnpm install`
4. do a basic build: `pnpm run build`
5. check configs\eslint-config\src\rules\eslint.ts, change no-cycle config, then `pnpm run --filter @easy-config/eslint-config build`
6. `pnpm run --filter @easy-proj/webapp lint` with `TIMING=0`
# Limits

I don't have a large enough project to benchmark the performance, so I'm not sure whether the conclusion above would still hold for very large projects.

Contributor guide

Open the contributing guide

Research direction

Start with the no-cycle configuration and implementation entry points referenced by the issue, using configs\eslint-config\src\rules\eslint.ts and the provided easy-projs reproduction. Compare the listed disableScc and ignoreExternal runs, then verify that ignoreExternal is effective with disableScc false without traversing the reported node_modules files and that the no-cycle timing improves.

Written by the indexing model from the issue text.

Assessment

Tech stack
eslint, javascript
Domain
performance, tooling
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Quiet
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.