jsx-eslint / eslint-plugin-jsx-a11y

Static AST checker for a11y rules on JSX elements.
MIT License
3.39k stars 637 forks source link

[New]: Add no-duplicate-ids lint rule #955

Open chrisrng opened 1 year ago

chrisrng commented 1 year ago

Enforces that no id attributes are reused. This rule does a basic check to ensure that id attribute values are not the same. In the case of a JSX expression, it checks that no id attributes reuse the same expression.

Open questions:

codecov[bot] commented 1 year ago

Codecov Report

Merging #955 (12ba068) into main (fffb05b) will increase coverage by 0.00%. The diff coverage is 100.00%.

:exclamation: Current head 12ba068 differs from pull request most recent head 837e891. Consider uploading reports for the commit 837e891 to get more accurate results

@@           Coverage Diff           @@
##             main     #955   +/-   ##
=======================================
  Coverage   99.02%   99.03%           
=======================================
  Files         105      106    +1     
  Lines        1642     1658   +16     
  Branches      578      581    +3     
=======================================
+ Hits         1626     1642   +16     
  Misses         16       16           
Files Coverage Δ
src/index.js 100.00% <ø> (ø)
src/rules/no-duplicate-ids.js 100.00% <100.00%> (ø)
ljharb commented 1 year ago

This would only be valuable for single files where an ID is duplicated, which seems like a very minority case - the common case here will be an ID that's mentioned once per file, but across multiple files.

id attributes are exceedingly rare, so this doesn't seem like that useful a rule.

chrisrng commented 1 year ago

@ljharb thanks for reviewing! We've seen that, when branching code due to AB tests, this can happen quite a lot due to copy paste and not noticing an if clause somewhere. The rule doesn't catch when it's used across files but it catches those cases. Would you consider adding this to the plugin if we don't add it to the recommended config?

ljharb commented 1 year ago

Nothing can be added to the recommended config without it being a breaking change, so that's not an option regardless.

This is something best discussed in an issue before writing any code, for future reference, but now that it's a PR let's discuss here.

Can you share some examples? I've seen lots of a/b tests in a react codebase and never run into this problem.

chrisrng commented 1 year ago

@ljharb Sorry will open an issue next time!

Basically something like this (this is example code with errors):

<div>
  <label htmlFor="input1">Username:</label>
  <input type="text" id="input1" name="username" />

  {emailIsEnabled ? (
    <div>
      <label htmlFor="input1">Email:</label>
      <input type="email" id="input1" name="email" />
    </div>
  ) : null}

  {passwordIsEnabled ? (
    <div>
      <label htmlFor="input1">Password:</label>
      <input type="password" id="input1" name="password" />
    </div>
  ) : null}
</div>

Also when dealing with aria-labelledby