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
Consider trimming the plugins entries in .eslintrc #6854
Comments
Actually that was silly of me to have you report this as a bug, it's really a core enhancement request. My bad. For the team: We should see if we can trim plugin entries, maybe "extends" entries as well, assuming whitespace is invalid on an npm package name. |
I'm not sure trimming is the right solution here. What was the output you got from ESLint? (Please paste in the complete output) |
I created a repository in case anyone wants to experience this for himself/herself: https://github.com/platinumazure/why-trim-plugins Output when running ESLint normally (without Oops! Something went wrong! :( ESLint couldn't find the plugin "eslint-plugin-qunit ". This can happen for a couple different reasons: 1. If ESLint is installed globally, then make sure eslint-plugin-qunit is also installed globally. A globally-installed ESLint cannot find a locally-installed plugin. 2. If ESLint is installed locally, then it's likely that the plugin isn't installed correctly. Try reinstalling by running the following: npm i eslint-plugin-qunit @latest --save-dev If you still can't figure out the problem, please stop by https://gitter.im/eslint/eslint to chat with the team. And here is my output when running with Sat, 13 Aug 2016 15:24:51 GMT eslint:cli Running on files Sat, 13 Aug 2016 15:24:51 GMT eslint:ignored-paths Looking for ignore file in C:\Users\kpartington\Documents\GitHub\why-trim-plugins Sat, 13 Aug 2016 15:24:51 GMT eslint:ignored-paths Could not find ignore file in cwd Sat, 13 Aug 2016 15:24:51 GMT eslint:glob-util Creating list of files to process. Sat, 13 Aug 2016 15:24:51 GMT eslint:cli-engine Processing C:\Users\kpartington\Documents\GitHub\why-trim-plugins\index.js Sat, 13 Aug 2016 15:24:51 GMT eslint:cli-engine Linting C:\Users\kpartington\Documents\GitHub\why-trim-plugins\index.js Sat, 13 Aug 2016 15:24:51 GMT eslint:config Constructing config for C:\Users\kpartington\Documents\GitHub\why-trim-plugins\index.js Sat, 13 Aug 2016 15:24:51 GMT eslint:config Using .eslintrc and package.json files Sat, 13 Aug 2016 15:24:51 GMT eslint:config Loading C:\Users\kpartington\Documents\GitHub\why-trim-plugins\.eslintrc.json Sat, 13 Aug 2016 15:24:51 GMT eslint:config-file Loading JSON config file: C:\Users\kpartington\Documents\GitHub\why-trim-plugins\.eslintrc.json Sat, 13 Aug 2016 15:24:51 GMT eslint:plugins Failed to load plugin eslint-plugin-qunit . Proceeding without it. Oops! Something went wrong! :( ESLint couldn't find the plugin "eslint-plugin-qunit ". This can happen for a couple different reasons: 1. If ESLint is installed globally, then make sure eslint-plugin-qunit is also installed globally. A globally-installed ESLint cannot find a locally-installed plugin. 2. If ESLint is installed locally, then it's likely that the plugin isn't installed correctly. Try reinstalling by running the following: npm i eslint-plugin-qunit @latest --save-dev If you still can't figure out the problem, please stop by https://gitter.im/eslint/eslint to chat with the team. My take is that the error is just possible to spot, if you're looking carefully. But I still think we could possibly do better. Trimming the plugin names before searching seems like a relatively safe approach to me. A more general approach, though, would be to run something like |
Thanks @platinumazure. Maybe even a warning if the trimmed plugin name doesn't match the given plugin name. I understand the implications of just having it silently trimmed and how that could cause a mess in the configuration file. |
I definitely don't want to trim the value, I'd rather encourage people to fix incorrect configs. Maybe we can just do a regex validation on the package name and throw up a pretty message if it fails validation. |
That's more less what I'm after overall. I'm sure it's not something that most people run across often so it could be easily overlooked with the generic message as seen above. I think the most troubling part is that it tells you to try and install using |
@jostrander do you want to submit a PR for this? |
Are you considering appending a message to eslint/messages/plugin-missing.txt or something more in depth which does the regex check for valid package names? |
Either append it there, or conditionally append an error message to the not found message here if the plugin name trimmed is different then the plugin name? |
I'll defer to @nzakas on the final answer. But if it were up to me, I would want the check done before even trying to load any plugins, and I would want a new message in eslint/messages to show the exact problem. |
I think what @platinumazure suggest is fine. We do need TSC approval to mark this issue as accepted though. So adding correct label: Summary for TSC: |
TSC agreement has been reached. We should display a new error message when whitespaces are detected in plugin/config name. |
Was the resolution to specifically address whitespaces or any failed plugin load? That would also fix #6115 |
I think the decision was to show a special message for the case when whitespaces are detected, but looks like the fix for #6115 will be a check in the same spot in code as this one. |
Yes, this one is just for validating the name (which can be done before attempting to load). #6115 has to do with what happens after you try to load and it failed. |
Working on this |
EDIT: I'm okay with just doing whitespace here (as long as it's space/tab/newline via regex). I raised a question about whether we should only validate whitespace on the issue, or if we should try to just check for a valid package name. There's not a lot of information on npm but there's enough. https://docs.npmjs.com/files/package.json#name
Unfortunately, many of those are not very useful to us or would be hard to check accurately. Name starting with dot or underscore would only apply if the full package name is used in config. If we are confident that ESLint plugins were only supported after npm started forbidding uppercase letters, we could check that. And we could possibly check against non-URL-safe characters (which would probably take care of the whitespace problem). If we've accepted this issue as a whitespace check only, then we can certainly merge PR #6960 and I can open a separate issue later. |
What version of ESLint are you using?
3.2.2
Please show your full configuration:
What did you do? Please include the actual source code causing the issue.
Completely user error: took me about 45 minutes to figure out the appended space in
.eslintrc
was causing the plugin to not be found.Can we consider have the plugins list configured to trim the entered values and/or give a warning that it had to be trimmed before trying to resolve them?
p.s. sorry for emitting the extraneous bug report template sections, they didn't really apply because it's simply user error on my end and the only relevant code is the
.eslintrc
file above.The text was updated successfully, but these errors were encountered: