sindresorhus / eslint-plugin-unicorn

More than 100 powerful ESLint rules
MIT License
4.18k stars 361 forks source link

Rule proposal: `no-query-selector-all-zero` #1404

Open dangowans opened 3 years ago

dangowans commented 3 years ago

Avoid selecting an entire NodeList when only the first item is desired.

Fail

document.querySelectorAll("form")[0].addEventListener("submit", submitFunction);

Pass

document.querySelector("form").addEventListener("submit", submitFunction);
dangowans commented 3 years ago

Running the recommended ruleset over my code base, I'm finding code like this:

containerEle.getElementsByTagName("form")[0].addEventListener("submit", submitFunction);

The unicorn/prefer-query-selector rule transforms that line to:

containerEle.querySelectorAll("form")[0].addEventListener("submit", submitFunction);

It's valid, but I don't think it's best. A rule like the one in this proposal would "finish the job".

containerEle.querySelector("form").addEventListener("submit", submitFunction);
sindresorhus commented 3 years ago

The unicorn/prefer-query-selector rule transforms that line to:

I think we should fix the fixer there regardless of whether this rule is accepted.

sindresorhus commented 3 years ago

Another fail:

document.querySelectorAll("form").at(0).addEventListener("submit", submitFunction);
sindresorhus commented 3 years ago

I wonder if there are other anti-patterns with querySelectorAll we could catch and then make this rule more general and named no-incorrect-query-selector or something.

dangowans commented 3 years ago

I wonder if there are other anti-patterns with querySelectorAll we could catch and then make this rule more general and named no-incorrect-query-selector or something.

Maybe if the selector ends with :first-child, but then again, you'd probably have to use index 0 to get the element anyway.

sindresorhus commented 3 years ago

Some ideas:


document.querySelectorAll('#foo')

Since there can only be a single ID.


const elements = document.querySelectorAll('.class');
if (elements) {

}

Forgetting to check the .length.


document.querySelectorAll('[data-foo=2]')

The number needs to be quoted.


And maybe detect some incorrect or inefficient selectors.

dangowans commented 3 years ago

document.querySelectorAll('#foo')

Definitely like this one.

document.querySelectorAll('[data-foo=2]')

Not sure if this one has the same goal of avoiding selecting a single element with querySelectorAll. Possibly a second rule like prefer-quoted-selector-attributes.

dangowans commented 3 years ago

If we're considering inefficient selectors, something like this may qualify.

document.querySelectorAll("select").querySelectorAll("option")

Could be better written as:

document.querySelectorAll("select option")
fisker commented 3 years ago

Actually, these two are different, the first one will result all <option>s even it's not in <select>.

There are difference between document.querySelectorAll("select").querySelectorAll("option") and document.querySelectorAll("select").querySelectorAll(":scope option")

dangowans commented 3 years ago
document.querySelectorAll("select").querySelectorAll("option")

Actually what I suggested doesn't work at all since you can't "double up" the querySelectorAll(). (Must have been tired.)

I think what I was thinking was something like this:

document.querySelector("#select").querySelectorAll("option")

Could be:

document.querySelector("#select option")

I feel like simplifying selectors is a HUGE task.

fregante commented 2 years ago
document.querySelectorAll('[data-foo=2]')

The number needs to be quoted.

I'm not sure you'd want to venture into validating CSS selectors, but if you do, this would probably deserve its own rule or even its own plugin.