Consider trimming the plugins entries in .eslintrc #6854

Closed
jostrander opened this Issue Aug 5, 2016 · 17 comments

Projects

None yet

7 participants

@jostrander
Contributor

What version of ESLint are you using?

3.2.2

Please show your full configuration:

{
  "plugins": [ "html " ],
  "extends": "airbnb-base"
}

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.

@eslintbot eslintbot added the triage label Aug 5, 2016
@platinumazure
Member

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.

@nzakas
Member
nzakas commented Aug 13, 2016

I'm not sure trimming is the right solution here. What was the output you got from ESLint? (Please paste in the complete output)

@platinumazure
Member
platinumazure commented Aug 13, 2016 edited

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 --debug):

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 --debug (having combined stderr and stdout into one file):

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 npm ls --depth=0 and do a string-difference on anything starting with "eslint-plugin" and making suggestions based on that.

@jostrander
Contributor

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.

@nzakas
Member
nzakas commented Aug 15, 2016

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.

@jostrander
Contributor

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 npm i eslint-plugin-qunit @latest --save-dev where the package name is correctly trimmed.

@nzakas
Member
nzakas commented Aug 15, 2016

@jostrander do you want to submit a PR for this?

@jostrander
Contributor

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?

@jostrander
Contributor

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?

@platinumazure
Member

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.

@ilyavolodin
Member

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:
Add additional error messages for when plugins/configs include whitespaces to make it easier to find problems.

@ilyavolodin ilyavolodin added accepted and removed evaluating labels Aug 18, 2016
@ilyavolodin
Member

TSC agreement has been reached. We should display a new error message when whitespaces are detected in plugin/config name.

@ilyavolodin ilyavolodin removed the tsc agenda label Aug 18, 2016
@nzakas nzakas added the beginner label Aug 19, 2016
@alberto
Member
alberto commented Aug 21, 2016

Was the resolution to specifically address whitespaces or any failed plugin load? That would also fix #6115

@vitorbal
Member

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.

@nzakas
Member
nzakas commented Aug 22, 2016

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.

@jostrander
Contributor

Working on this

@jostrander jostrander pushed a commit to jostrander/eslint that referenced this issue Aug 22, 2016
= Update: Throw error if whitespace found in plugin name (fixes #6854) dffb2df
@platinumazure
Member
platinumazure commented Aug 23, 2016 edited

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

Some rules:

  • The name must be less than or equal to 214 characters. This includes the scope for scoped packages.
  • The name can't start with a dot or an underscore.
  • New packages must not have uppercase letters in the name.
  • The name ends up being part of a URL, an argument on the command line, and a folder name. Therefore, the name can't contain any non-URL-safe characters.

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.

@nzakas nzakas closed this in be29599 Sep 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment