eslint-plugin-unicorn icon indicating copy to clipboard operation
eslint-plugin-unicorn copied to clipboard

Add `unopinionated` preset

Open kripod opened this issue 4 years ago • 17 comments

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"
  ]
}

kripod avatar Oct 30 '20 00:10 kripod

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.

sindresorhus avatar Nov 02 '20 22:11 sindresorhus

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 avatar Nov 02 '20 22:11 kripod

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

kripod avatar Nov 21 '20 20:11 kripod

@fisker Are you ok with this?

sindresorhus avatar Nov 22 '20 02:11 sindresorhus

Yes, I think it's good.

fisker avatar Nov 22 '20 03:11 fisker

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

fregante avatar Apr 20 '21 17:04 fregante

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.

sindresorhus avatar Apr 20 '21 18:04 sindresorhus

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

osdiab avatar May 27 '21 00:05 osdiab

@osdiab https://github.com/prettier/eslint-config-prettier/blob/83c4f374f06d0cb1c0e55bc3f3b1ab75a1a34a89/index.js#L142-L144

fisker avatar May 27 '21 00:05 fisker

Ah I was on an old version, thanks for this!

Omar

On Thu, May 27, 2021 at 9:58 AM, fisker Cheung @.***> wrote:

@osdiab https://github.com/osdiab https://github.com/prettier/ eslint-config-prettier/blob/83c4f374f06d0cb1c0e55bc3f3b1ab75a1a34a89/ index.js#L142-L144

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sindresorhus/eslint-plugin-unicorn/issues/896#issuecomment-849228606, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAONU3XEAUVQPYLM5UWPMVTTPWKLTANCNFSM4TEMYEDA .

osdiab avatar May 27 '21 05:05 osdiab

Hey guys, don't trust the guy, you are doing a great job, don:t reduce all to be kind of an abbreviation.

quirimmo avatar Nov 09 '21 14:11 quirimmo

dont really waste time replying to such comments, please focus working on amazing rules like no set duplicates

quirimmo avatar Nov 09 '21 14:11 quirimmo

another point: how would you suggest me to loop over a list without using alll native methods? shall I perhaps access statically all the member by indexing?

quirimmo avatar Nov 09 '21 14:11 quirimmo

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.

andersk avatar Nov 26 '21 03:11 andersk

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',
}

levino avatar Sep 30 '22 05:09 levino

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

RicardoValdovinos avatar Jul 28 '23 18:07 RicardoValdovinos

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

sindresorhus avatar Feb 22 '24 06:02 sindresorhus