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

Treat package.json just like .eslintrc files #4451

Closed
nzakas opened this Issue Nov 16, 2015 · 13 comments

Comments

Projects
None yet
4 participants
@nzakas
Copy link
Member

nzakas commented Nov 16, 2015

When we first added package.json support, we made the decision that if there was a package.json and an .eslintrc file in the same directory, that we would use them both and combine the configuration. While it seemed like a good idea at the time, this setup has created some complexity in the calculation of configs that doesn't useful or worthwhile. It's unlikely that people are ever going to use both package.json and .eslintrc, and it would simplify the code to treat them the same.

I'd like to propose that package.json be treated just like .eslintrc (and all of the versions with extensions, .eslintrc.js, etc.) in that if present, it will be the only config file used. It would have highest priority to mimic the current behavior and hopefully cause as few issues as possible.

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Nov 16, 2015

Sounds good to me. 👍

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Nov 16, 2015

Agree.

@ilyavolodin ilyavolodin added accepted and removed evaluating labels Nov 16, 2015

@nzakas nzakas self-assigned this Nov 16, 2015

@nzakas nzakas added this to the v2.0.0 milestone Nov 16, 2015

@nzakas nzakas referenced this issue Nov 16, 2015

Closed

Roadmap: 2.0.0 #3561

11 of 11 tasks complete

@nzakas nzakas changed the title Proposal: Treat package.json just like .eslintrc files Treat package.json just like .eslintrc files Dec 1, 2015

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 5, 2015

Small correction to the proposal above. package.json should only be used as a top level config file if it contains eslintConfig property, otherwise it should be discounted from the list of order of config files.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 5, 2015

The idea of this change is to simplify the code and make it work the same way for all configuration files, however, correction that I added above actually ends up making code more complicated then it is now. Now file-finder actually has to be able to parse package.json file and verify that it has eslintConfig property before bailing out and saying that package.json file that should be used for configuration. I've been looking at this for a little while, and I'm not sure this change is worth making.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Dec 6, 2015

Hmmm good point. I'll take a closer look when I can.

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Dec 6, 2015

Ah, how about this: if we make package.json the lowest priority, then we don't need to modify file Finder. Just use ConfigFile.load() and see if the return value is null. Since it will only be checked if no other standard config files are found, it should be fairly straightforward.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 6, 2015

That would work, but I'm not sure if it makes sense from the user's perspective

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Dec 6, 2015

Because?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 6, 2015

I don't know, personally for me, package.json seems like a top level configuration that should override everything else. But that's only my personal feeling, don't know if it applies to anyone else.

ilyavolodin added a commit that referenced this issue Dec 7, 2015

ilyavolodin added a commit that referenced this issue Dec 7, 2015

ilyavolodin added a commit that referenced this issue Dec 7, 2015

@nzakas

This comment has been minimized.

Copy link
Member Author

nzakas commented Dec 7, 2015

The goal here is really to eliminate the special case of merging .eslintrc files with package.json files in the same directory. Whichever is higher priority really doesn't matter unless you have two files already, in which case you'll need to combine the configs into one file.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 7, 2015

Agree. I opened a PR with package.json at lowest priority. Even though it's still a bit confusing to me personally, but I think there's no default expected behavior for package.json config section, so people will be able to learn about priority from the documentation.

@platinumazure

This comment has been minimized.

Copy link
Member

platinumazure commented Dec 7, 2015

Do we know that a lot of people used merged configs? Speaking at least for myself, I know I would find that (i.e., having half my config in package.json and the other half in .eslintrc) confusing as hell.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Dec 7, 2015

@platinumazure We do not know, but we assume that not a lot of people do. That's why we are removing it.

ilyavolodin added a commit that referenced this issue Dec 7, 2015

nzakas added a commit that referenced this issue Dec 8, 2015

Merge pull request #4624 from eslint/issue-4451
Breaking: Treat package.json like the rest of configs (fixes #4451)

@eslint eslint bot locked and limited conversation to collaborators Feb 7, 2018

@eslint eslint bot added the archived due to age label Feb 7, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.