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

Consider trimming the plugins entries in .eslintrc #6854

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

Consider trimming the plugins entries in .eslintrc #6854

jostrander opened this issue Aug 5, 2016 · 17 comments
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint good first issue Good for people who haven't worked on ESLint before

Comments

@jostrander
Copy link
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 An ESLint team member will look at this issue soon label Aug 5, 2016
@platinumazure
Copy link
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.

@platinumazure platinumazure added enhancement This change enhances an existing feature of ESLint core Relates to ESLint's core APIs and features evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Aug 5, 2016
@nzakas
Copy link
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
Copy link
Member

platinumazure commented Aug 13, 2016

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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Member

nzakas commented Aug 15, 2016

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

@jostrander
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
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 ilyavolodin added the tsc agenda This issue will be discussed by ESLint's TSC at the next meeting label Aug 15, 2016
@ilyavolodin
Copy link
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 There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Aug 18, 2016
@ilyavolodin
Copy link
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 This issue will be discussed by ESLint's TSC at the next meeting label Aug 18, 2016
@nzakas nzakas added the good first issue Good for people who haven't worked on ESLint before label Aug 19, 2016
@alberto
Copy link
Member

alberto commented Aug 21, 2016

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

@vitorbal
Copy link
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
Copy link
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
Copy link
Contributor Author

Working on this

@platinumazure
Copy link
Member

platinumazure commented Aug 23, 2016

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 as completed in be29599 Sep 3, 2016
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion core Relates to ESLint's core APIs and features enhancement This change enhances an existing feature of ESLint good first issue Good for people who haven't worked on ESLint before
Projects
None yet
Development

No branches or pull requests

7 participants