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

Add unopinionated preset #896

Open
kripod opened this issue Oct 30, 2020 · 17 comments
Open

Add unopinionated preset #896

kripod opened this issue Oct 30, 2020 · 17 comments

Comments

@kripod
Copy link
Contributor

kripod commented Oct 30, 2020

I propose turning off the following rules by default, so that the recommended config could act as a baseline which may be extended explicitly:

  • unicorn/filename-case – This goes against the principle set by one of the most popular linting packages, eslint-config-airbnb. See this excerpt from their style guide:

    23.6 A base filename should exactly match the name of its default export.

  • unicorn/no-nested-ternary – Doesn't work properly when used along with Prettier, as formatting removes parentheses used in ternary expressions.

  • unicorn/no-null – This is very opinionated and external libraries like React and Prisma depend on null. Also, something == null checks for both null and undefined at once, so it's less prone to errors in comparison to something === undefined || something === null.

  • unicorn/no-useless-undefined – Conflicts with ESLint's built-in consistent-return rule, which is enforced by Airbnb's config. Causes all of the following code to be invalid:

    function fetchModel() {
      if (exists) return { something: "some data" };
      return undefined; // As embraced by `unicorn/no-null`
    }
    
    function fetchModel() {
      if (exists) return { something: "some data" };
      return; // Violates `consistent-return`, even when omitted 
    }
  • unicorn/prefer-query-selector – The alternatives, namely getElementById and getElementsByClassName, offer better performance. Selector-based queries should be avoided in general, even in CSS, as they get complex pretty fast. Using IDs and class names for locating elements is a well-tested practice, while complex selectors are questionable.

  • unicorn/prevent-abbreviations – While the intent is great, way too many false positives may occur. For instance, React uses the term props to identify attributes passed to a JSX element. They aren't widely called properties in the documentation and tutorials. Same goes for the req and res parameters of an API handler function, as popularized by Express.

As an alternative, a recommended-loose or recommended-unopinionated config may also be added to avoid the issues outline above. My goal is to set up ESLint with a strict set of rules as simple as below:

{
  "root": true,
  "parserOptions": { "project": "./tsconfig.json" },
  "extends": [
    "airbnb-typescript",
    "airbnb/hooks",
    "plugin:@typescript-eslint/recommended",
    "plugin:unicorn/recommended",
    "prettier",
    "prettier/@typescript-eslint",
    "prettier/react",
    "prettier/unicorn"
  ]
}
@sindresorhus
Copy link
Owner

I'm open to adding an unopinionated config, but let's see what the other maintainers here think. I don't think it should include the word recommended though, as it's not actually something we recommend.


unicorn/no-nested-ternary – Doesn't work properly when used along with Prettier, as formatting removes parentheses used in ternary expressions.

This is not about being opinionated though. We are not going to leave out rules just because they don't work with Prettier. This is already handled by https://github.com/prettier/eslint-config-prettier/blob/07380e2235cea405822b7c0de20bbecbf2dc6da0/unicorn.js#L5.


unicorn/no-nested-ternary – Doesn't work properly when used along with Prettier, as formatting removes parentheses used in ternary expressions.

We already handle some React cases and we're happy to handle more, but yes, this one is quite opinionated.


unicorn/prefer-query-selector – The alternatives, namely getElementById and getElementsByClassName, offer better performance. Selector-based queries should be avoided in general, even in CSS, as they get complex pretty fast. Using IDs and class names for locating elements is a well-tested practice, while complex selectors are questionable.

You can select classes and IDs with query selector too. I remember reading something about engines just using the fast-path internally for simple selectors.


While the intent is great, way too many false positives may occur. For instance, React uses the term props to identify attributes passed to a JSX element. They aren't widely called properties in the documentation and tutorials. Same goes for the req and res parameters of an API handler function, as popularized by Express.

I can buy the React argument as there are many places you have to use the props wording (Although, here too we have workarounds for React). But in the case of Express, it just leads to less readable code.

@kripod
Copy link
Contributor Author

kripod commented Nov 2, 2020

I would highly appreciate a separate unopinionated or loose config if that’s a possibility. Thanks for taking your time to read and review my suggestions! 🙏

@kripod
Copy link
Contributor Author

kripod commented Nov 21, 2020

@sindresorhus Is there anything I could help with to advance the state of this proposal?

@sindresorhus
Copy link
Owner

@fisker Are you ok with this?

@fisker
Copy link
Collaborator

fisker commented Nov 22, 2020

Yes, I think it's good.

@sindresorhus sindresorhus changed the title Remove opinionated rules from the recommended config Add unopinionated preset Jan 1, 2021
@fregante
Copy link
Collaborator

Isn't every rule potentially "opinionated"? I think you're looking at a airbnb-specific unicorn configuration, like a eslint-config-airbnb-unicorn package like the many that exist for XO: https://github.com/xojs/xo#configs

Or, if worth having in this package, perhaps it should explicitly mention airbnb: plugin:unicorn/airbnb

@sindresorhus
Copy link
Owner

Isn't every rule potentially "opinionated"?

No. Some rules are about correctness or preventing bugs. I don't consider that opinionated. I also wouldn't consider rules that enforce using more modern APIs opinionated.

For example, no-instanceof-array is about correctness, but no-array-reduce is opinionated.

Or, if worth having in this package, perhaps it should explicitly mention airbnb: plugin:unicorn/airbnb

I don't see why it should have the name of a random ESLint config. unopinionated is perfectly clear.

@osdiab
Copy link

osdiab commented May 27, 2021

Would also be nice if there were some preset that disables the rules that conflict with prettier, as that was pretty annoying.

@osdiab
Copy link

osdiab commented May 27, 2021 via email

@quirimmo

This comment has been minimized.

@quirimmo

This comment has been minimized.

@quirimmo

This comment has been minimized.

@andersk
Copy link
Contributor

andersk commented Nov 26, 2021

You can select classes and IDs with query selector too. I remember reading something about engines just using the fast-path internally for simple selectors.

Evidence to the contrary, showing a significant performance advantage for getElementById("foo") over querySelector("#foo") in both Firefox and Chrome:

https://blog.wesleyac.com/posts/getelementbyid-vs-queryselector

Also, correctly selecting a computed ID or class name with querySelector may require a CSS.escape.

@levino
Copy link

levino commented Sep 30, 2022

If someon wants to switch off the rules from OP, here is a snippet to copy and paste:

{
  'unicorn/prevent-abbreviations': 'off',
  'unicorn/filename-case': 'off',
  'unicorn/no-nested-ternary': 'off',
  'unicorn/no-null': 'off',
  'unicorn/no-useless-undefined': 'off',
  'unicorn/prefer-query-selector': 'off',
}

@RicardoValdovinos
Copy link

Hello, wondering if there were any updates on an unopinionated config setting?

@sindresorhus
Copy link
Owner

We will add this when we have fully switched to flat config. I don't want to expose more configs until then.

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

Successfully merging a pull request may close this issue.

9 participants