Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed up no-relative-import rule #21

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

snowystinger
Copy link

First, thank you for these rules, they've caught many issues before they became issues.
I work on a very large monorepo https://github.com/adobe/react-spectrum and our eslint has slowed down substantially.
Eslint performance today:

Rule                                        | Time (ms) | Relative
:-------------------------------------------|----------:|--------:
monorepo/no-relative-import                 |  6535.571 |    48.0%
no-redeclare                                |   614.720 |     4.5%
rulesdir/imports                            |   547.905 |     4.0%
@typescript-eslint/no-unused-vars           |   436.368 |     3.2%
jsdoc/require-description-complete-sentence |   435.446 |     3.2%
indent-legacy                               |   286.457 |     2.1%
react/jsx-indent                            |   251.766 |     1.8%
jsdoc/check-alignment                       |   249.314 |     1.8%
jsdoc/check-indentation                     |   224.696 |     1.6%
jsdoc/check-tag-names                       |   221.656 |     1.6%

I put together this PR in order to improve our overall runtime.
This does two things to speed up performance of this rule:

  1. minimatch is a glob expressions conversion into JavaScript RegExp, this is useful for complex things, however, the way it's used here is minimatch(filePath, path.join(pkg.location, '**'))
    this would lead to a glob of ${pkg.location}/** and we want to know if filePath matches that glob
    this is the same as isInside, which is faster
    overall though the time saved isn't that big, but ~7% of a several minute run is nothing to be sad about

  2. this rule only cares about relative imports, I may be wrong about this, but I believe all relative imports will have '../' in them, therefore we can shortcut the entire rule if that isn't in the import path
    this is where the big time savings came from

After minimatch replacement:

Rule                                        | Time (ms) | Relative
:-------------------------------------------|----------:|--------:
monorepo/no-relative-import                 |  5251.391 |    41.3%
no-redeclare                                |   638.222 |     5.0%
rulesdir/imports                            |   518.794 |     4.1%
jsdoc/require-description-complete-sentence |   460.373 |     3.6%
@typescript-eslint/no-unused-vars           |   444.902 |     3.5%
indent-legacy                               |   317.514 |     2.5%
react/jsx-indent                            |   264.220 |     2.1%
jsdoc/check-tag-names                       |   257.819 |     2.0%
jsdoc/check-alignment                       |   257.492 |     2.0%
jsdoc/check-indentation                     |   237.808 |     1.9%

After '../' shortcut:

Rule                                        | Time (ms) | Relative
:-------------------------------------------|----------:|--------:
no-redeclare                                |   698.225 |     9.2%
monorepo/no-relative-import                 |   480.147 |     6.3%
rulesdir/imports                            |   477.050 |     6.3%
@typescript-eslint/no-unused-vars           |   456.465 |     6.0%
jsdoc/require-description-complete-sentence |   416.341 |     5.5%
indent-legacy                               |   287.541 |     3.8%
react/jsx-indent                            |   273.187 |     3.6%
jsdoc/check-indentation                     |   258.600 |     3.4%
jsdoc/check-tag-names                       |   252.098 |     3.3%
jsdoc/check-alignment                       |   232.608 |     3.1%

snowystinger added a commit to adobe/react-spectrum that referenced this pull request Apr 19, 2021
Patching eslint-plugin-monorepo based on a PR I opened to that repo, you can see details here azz/eslint-plugin-monorepo#21
devongovett pushed a commit to adobe/react-spectrum that referenced this pull request Apr 19, 2021
Patching eslint-plugin-monorepo based on a PR I opened to that repo, you can see details here azz/eslint-plugin-monorepo#21
majornista pushed a commit to majornista/react-spectrum-v3 that referenced this pull request May 19, 2021
Patching eslint-plugin-monorepo based on a PR I opened to that repo, you can see details here azz/eslint-plugin-monorepo#21
@snowystinger
Copy link
Author

Just a friendly bump :)

@snowystinger
Copy link
Author

another friendly bump :) would love to get rid of our patch-package

@jdb8
Copy link

jdb8 commented Nov 19, 2021

FWIW I published a fork at @jdb8/eslint-plugin-monorepo to npm if it's useful: https://github.com/jdb8/eslint-plugin-monorepo

It contains this PR and #29, which should provide some additional perf improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants