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

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

chrisrng
Copy link

@chrisrng chrisrng commented Oct 4, 2023

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:

  • Adding as recommended since it's a WCAG violation is there any steps for bumping a minor version or something?
  • Does the JSX expression check make sense or we can just remove it?

@codecov
Copy link

codecov bot commented Oct 4, 2023

Codecov Report

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

❗ 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%> (ø)

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.
@ljharb
Copy link
Member

ljharb commented Oct 5, 2023

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.

@ljharb ljharb marked this pull request as draft October 5, 2023 11:38
@chrisrng
Copy link
Author

chrisrng commented Oct 5, 2023

@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
Copy link
Member

ljharb commented Oct 5, 2023

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
Copy link
Author

chrisrng commented Oct 5, 2023

@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

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

Successfully merging this pull request may close these issues.

2 participants