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

New: Plugin Loading Improvement #47

Merged
merged 7 commits into from
Dec 14, 2019
Merged

Conversation

mysticatea
Copy link
Member

@mysticatea mysticatea commented Nov 7, 2019

Summary

This RFC changes ESLint to load plugins from the directory of the config files, so it fixes "plugin not found" errors for varied environments.

Related Issues

@mysticatea mysticatea added Initial Commenting This RFC is in the initial feedback stage breaking change This RFC contains breaking changes labels Nov 7, 2019
Copy link
Member

@ilyavolodin ilyavolodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

General question. Why wouldn't we just use require.resolve for each plugin loaded in a given config file from the location of the config file? Unless --resolve-plugins-relative-to is provided.

@mysticatea
Copy link
Member Author

mysticatea commented Nov 12, 2019

True, this proposal intends to just use require.resolve to load each plugin from each config file.

But I thought we should warn confliction. For example, if there are the plugin foo in both ./node_modules/eslint-plugin-foo and ./subdir/node_modules/eslint-plugin-foo, then ./.eslintrc and ./subdir/.eslintrc load them, then it causes inunderstandable errors by the confliction.

@ilyavolodin
Copy link
Member

Got it. Thanks for clarification. To be honest, I still think we are creating artificial constraints for ourselves. The whole scenario of using same plugin from multiple places/multiple versions doesn't seem like a real-world issue to me. I'm sure it will happen, but I'm also reasonably sure that it will be very rare and putting those constrains in on every design seems like an overkill to me.

@mysticatea
Copy link
Member Author

I believe to print understandable errors are worthful regardless of the frequency, especially if the raw error is too difficult to find the cause.

@kaicataldo
Copy link
Member

This is more a general topic, but I tend to agree with @ilyavolodin. While I agree that helpful error messages should be helpful, I don't think it should be a higher priority than an intuitive user experience.

@mysticatea
Copy link
Member Author

Indeed, the shadowing that this RFC describes will not happen frequently. But if it happened, the error will appear as an "invalid option" or "rule not found" error. Please image that people opens an issue with the error. The error is too far from the cause, so I think that the validation in this RFC is worthful.

@ilyavolodin
Copy link
Member

@mysticatea Oh, absolutely, I think we are on the same page on this. We should provide helpful error messages to point users to the correct solution (although, I can't say that it's been working out well so far). So just to clarify this RFC suggests using require.resolve to load plugins, but also provide meaningful error messages when plugin failed to load, correct?

@mysticatea
Copy link
Member Author

Thank you for your feedback.

  • I have updated this RFC largely to clarify. Now, this proposal clearly aims at loading plugins like require.resolve. Then it describes the plugin uniqueness in varied situations.
  • I have changed about handling the --resolve-plugin-relative-to option. Now we can use the --resolve-plugin-relative-to option to rollback this change if a user needed.
  • I have added about implementation. It will be simple changes.

Copy link
Member

@ilyavolodin ilyavolodin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks good. One question about performance.

@mysticatea mysticatea added Final Commenting This RFC is in the final week of commenting and removed Initial Commenting This RFC is in the initial feedback stage labels Dec 4, 2019
@mysticatea mysticatea merged commit bcd9b41 into master Dec 14, 2019
@mysticatea mysticatea deleted the plugin-loading-improvement branch December 14, 2019 05:02
mysticatea added a commit to eslint/eslint that referenced this pull request Feb 16, 2020
mysticatea added a commit to eslint/eslint that referenced this pull request Feb 16, 2020
mysticatea added a commit to eslint/eslint that referenced this pull request Feb 16, 2020
mysticatea added a commit to eslint/eslint that referenced this pull request Feb 22, 2020
mysticatea added a commit to eslint/eslint that referenced this pull request Feb 22, 2020
kaicataldo added a commit to eslint/eslint that referenced this pull request Mar 27, 2020
* Breaking: change relative paths with --config (refs eslint/rfcs#37)

* update docs

* Breaking: improve plugin resolving (refs eslint/rfcs#47)

* replace getRules by getRulesForFile (refs eslint/rfcs#47)

* replace rulesMeta by getRuleMeta (refs eslint/rfcs#47)

* replace report.usedDeprecatedRules by report.results[].usedDeprecatedRules

* Revert "replace report.usedDeprecatedRules by report.results[].usedDeprecatedRules"

This reverts commit f3cc32f.

* Revert "replace rulesMeta by getRuleMeta (refs eslint/rfcs#47)"

This reverts commit 0d6afaf.

* Revert "replace getRules by getRulesForFile (refs eslint/rfcs#47)"

This reverts commit d29f613.

* update docs

* Update docs/user-guide/configuring.md

Co-Authored-By: Kai Cataldo <kai@kaicataldo.com>

* fix markdownlint error

Co-authored-by: Kai Cataldo <kai@kaicataldo.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This RFC contains breaking changes Final Commenting This RFC is in the final week of commenting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants