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

Provide "import:all" preset #551

Closed
nicolo-ribaudo opened this issue Sep 9, 2016 · 7 comments
Closed

Provide "import:all" preset #551

nicolo-ribaudo opened this issue Sep 9, 2016 · 7 comments

Comments

@nicolo-ribaudo
Copy link
Contributor

nicolo-ribaudo commented Sep 9, 2016

Some plugins (like "eslint-plugin-react") and ESLint itself provide a preset to enable every option, which is used to have the most strict configuration by default.
Can this plugin provide such preset?

Example:

{
  "extends": [ "eslint:all", "plugin:import/all" ],
  "rules": {
    // Unwanted rules needs to be disabled
    "import/no-mutable-exports": 0
   }
}
@nicolo-ribaudo nicolo-ribaudo changed the title Provide "import:all" preset INVALID Sep 9, 2016
@nicolo-ribaudo nicolo-ribaudo changed the title INVALID Provide "import:all" preset Sep 9, 2016
@nicolo-ribaudo nicolo-ribaudo reopened this Sep 9, 2016
@jfmengels
Copy link
Collaborator

Hi @nicolo-ribaudo!

Thanks for the proposal, though I'll have to say I'm not a big fan of this. I argued against it when the proposal was made in the ESLint repo.

For me, this brings a few problems:

  • As a user, your build can be broken at every release of ESLint or of a plugin whose all preset is used.
  • As a user, not configuring ESLint up front means that you will (probably) have bad surprises later on and you'll have to turn off some rules. You're increasing the chances of having to discuss your ESLint configuration with your team in a PR that has nothing to do with the purpose of the PR.
  • Every rule addition becomes a breaking change. The user can be warned about the unreliability of the preset in the documentation, but still.
  • It forces the maintainers to make on opinionated choice on the settings to turn on for rules that have them. In the case of this plugin, we might favor the import syntax on some rules while your project doesn't have access to it because you're not using Babel.

I'd recommend either copy-pasting the configuration in your config (for the core rules, this plugin doesn't do that) and trying that out (fast way), or to take some time to look at the rules and make some opinions up front on the rules to turn on (slow way). I'd recommend using tools like https://github.com/sarbbottam/eslint-find-rules/ to stay up to date with new rules.

@ljharb I saw that you participated in the discussion for the react plugin. Would you like to share your opinion on this?

@ljharb
Copy link
Member

ljharb commented Sep 11, 2016

These are all very good points. I generally think of an "all" config with anything turned on as exempt from semver-major rules, otherwise minor couldn't exist.

In a top-level app, your first two points don't apply, since naturally you'd be using shrinkwrap. In a reusable package, though, that's definitely a possibility when using ^. If that possibility bothers you, youd already need to use ~ with eslint based on their semver policy, so using it with all eslint plugins isn't unreasonable either.

Maintainers should be making opinionated choices on the settings (including keeping the defaults) imo, so that doesn't bother me.

I use eslint-find-rules --unused to gate prepublish, and it's great - but I don't think that should be required for someone who wants to include all rules for whatever reason.

To sum up, "all" config seems perfectly fine with appropriate documentation - users can get that semantic anyway by using eslint-find-rules programmatically in an eslint config JS file, and it seems silly to force that on the users who want it.

@benmosher
Copy link
Member

Great points from both @jfmengels and @ljharb.

I'm going to say no, at least for now. I agree with @ljharb that

Maintainers should be making opinionated choices on the settings

but there are a few reasons I don't want to do this, at least right now:

  • there are a lot of rules in here that are pretty opinionated, where I either don't share the opinion or, generally, the use case where it matters
  • I'm not positive every rule can coexist
  • several can enforce two polar opposite conditions (i.e. extensions) and I don't want to spend decision fatigue / bikeshed bucks picking some "official" stance

and, mostly:

  • I'm detecting a pattern where the existence of a lint rule implies some underlying best practice that is important to follow, and that the "cleanest" code would pass all existing lint rules

I think the all config plays into that last point, and I don't want to contribute to that.

It's arguable that given the conservative stance they take toward accepting rules, that ESLint's all config might be supported by some set of best-practice arguments.

Whether or not that's true, I'm pretty confident it's not true of this plugin. I do my best not to be too opinionated about what rules I accept from the community. If folks have use cases I don't, I'm fine to support them, but accepting a rule PR does not imply that I think everyone can/should use it.

Also, FWIW: anyone could build an publish a shared config with every rule turned on, and spend the time and energy to decide what the settings should be.

Though I'm tempted since there is a precedent in ESLint proper and other plugins, I don't think it makes sense for this plugin.

😄

@mockdeep
Copy link

Would love to have this as well. It's pretty extensively argued in the other thread, so won't rehash here, but you can see some of what I had to say on the topic here. As a user of your library, and knowing what I'm opting into, it's a pretty nice convenience to not have to go checking for new rules when I upgrade, and to not have to configure a bunch of already reasonable defaults. It compounds when you've got a number of linters integrated, such as Rubocop and SCSS Lint.

@ljharb
Copy link
Member

ljharb commented Feb 14, 2017

@mockdeep use eslint-find-rules --unused as a prepublish script, and you won't have to check for anything. See https://github.com/airbnb/javascript/blob/master/packages/eslint-config-airbnb-base/package.json#L10 for an example.

@mockdeep
Copy link

@ljharb Correct me if I'm wrong, but it looks like that configuration forces you to manually configure every single rule. My hope (and generally, experience) is that for the vast majority of rules, the defaults are reasonable, so I really just want to configure the rules that depart from the defaults in my configuration file, and for the rest enforce them as is.

@yyueniao
Copy link

yyueniao commented Oct 3, 2023

FWIW: anyone could build an publish a shared config with every rule turned on, and spend the time and energy to decide what the settings should be.

It's surprising that after 7 years, we still haven't seen this happen. In my opinion, what developers really need is not a rigid and conflict-free set of rules. Instead, we should enable all the rules, even if some of them shouldn't coexist, like no-default-export and prefer-default-export. Then, we can customize or disable them in the rules section based on specific use cases and preferences.

If the author still believes that implementing this isn't worthwhile, I have created a minimalist shared config called eslint-config-import-strict with a rule set named import-strict/all. This simply enables all rules without any additional configuration. Developers will need to customize it according to their own needs.

I am also contemplating the creation of an import-strict/strict rule set within this config, which would offer a more opinionated and less effort-intensive option for developers. However, I'd like to assess the developer experience and collect feedback before proceeding with that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

6 participants