-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
no-restricted-paths is slow when many imports/excepts #1758
Comments
How many exception paths do you have? O.o |
The line you referenced happens when |
Quite a few (but we're working on that), but what we have more of is files and imports - it's a large codebase, and I'm trying to use this rule and/or no-restricted-imports to help improve and enforce separation. I saw that |
The |
Yes, I don't see that it's used for this validation though. Looks like a validation of the rule configuration, that the Thanks for the quick responses, and apologies if I'm missing it. |
Just a thought - if we supported globs/stars in the exceptions, would that let you consolidate your list significantly? |
It does, but I don't see it ( const hasValidExceptionPaths = absoluteExceptionPaths
.every((absoluteExceptionPath) => isValidExceptionPath(absoluteFrom, absoluteExceptionPath)) It's only used later on L96-97 to check if an offending import is excepted: const pathIsExcepted = absoluteExceptionPaths
.some((absoluteExceptionPath) => containsPath(absoluteImportPath, absoluteExceptionPath)) Looking at it from another angle, it's the validation for this part of the config, which doesn't sound node-specific. I'll take your word for it though if I'm still not making sense.
Globs would help consolidate the list, yes (I see there's a PR #1708). Though I've gone with |
aha, i do see what you mean. it would split up the validation a bit, but a PR that hoists whatever can be hoisted out of the visitor into |
@ljharb I prepared a PR, could you please have a look when you have a chance? On the project I work on, it made the check an order of magnitude faster. |
Absolutely! Note that tests are currently broken on master (#1986) so i might not be able to land it just yet. |
I'm experiencing slow performance for this rule as well. You can see the configuration in: https://github.com/pixiebrix/pixiebrix-extension/blob/2604923b0f29c63bae5e1fbd690db4e33bd4b23c/.eslintrc.js#L15-L16 (note that dropping the glob only maybe saves 1 second) In short, I don't want files in
I think it might not actually be the specific rule though:
|
Indeed, if you maximally enable the useful import rules, you may find another rule is the 30s one. |
I disabled the rule, nothing else came close to it, so it's definitely one of the slowest ones. The other one is See comparison in: |
The rule appears to be building and validating the
excepts
for each zone every time an import is encountered. This makes it very slow when you have many files/imports, zones, and/or excepts.I narrowed it down to this validation part here:
https://github.com/benmosher/eslint-plugin-import/blob/master/src/rules/no-restricted-paths.js#L88
Could it instead validate the exception paths once when the rule is initialised?
The text was updated successfully, but these errors were encountered: