[feat] no-unnecessary-key should catch constant keys outside of map calls

[bschlenk]

Problem Description

It's a strange situation and I don't know why someone would write code this way, but I've ran into a few hardcoded string keys that are used outside of a map call. These are not eligible for the "Static keys used to re-mount components" exception the rule docs mention, since these keys are constant they would never trigger a re-mount.

It would be cool if this rule flagged constant keys like this since they meet the criteria for being unnecessary.

Alternative Solutions

I don't see any other rules in this project that would apply, although in my case the no-duplicate-keys rule did catch some that just happened to be duplicates but also strings.

Feature Details

This would just be an extension on top of the existing no-unnecessary-key rule and enabled by default.

Examples

<Menu.Root>
  <Menu.Option key="one">
    One
  </Menu.Option>
  <Menu.Option key="two">
    Two
  </Menu.Option>
</Menu.Root>

Both of these keys are unnecessary:

• they are not used in a map call
• they are constants so would never trigger a re-mount

Evaluation Checklist

• I have had problems that this feature would solve
• I could not find a way to solve the problem with existing features or workarounds
• I have thought very hard about potential edge cases and downsides, and they are acceptable
• I think the feature is well-defined and would provide clear value to users

[Rel1cx]

In this type of situation, the key might indeed be completely useless or accidentally set by the developer. However,

they are not used in a map call
they are constants, so would never trigger a re-mount

The assertions above are insufficient to conclude that the keys are useless. I did not spend much time constructing one that is not in a map call and also uses a constant string key, but can still correctly reset the subtree when needed:

import { useState } from "react";

function App() {
  const [lang, setLang] = useState<"en" | "fr" | "ar">("en");
  return (
    <div>
      <p>Current language: {lang}</p>
      <IResetChildrenOnWritingDirectionChange1 lang={lang}><input /></IResetChildrenOnWritingDirectionChange1>
      <IResetChildrenOnWritingDirectionChange2 lang={lang}><input /></IResetChildrenOnWritingDirectionChange2>
      <button onClick={() => setLang("en")}>English</button>
      <button onClick={() => setLang("fr")}>French</button>
      <button onClick={() => setLang("ar")}>Arabic</button>
    </div>
  );
}

// In this case, the elements with constant keys themselves do not change position in the JSX syntax tree,
// but conditional expressions (or control flows) select which one is present in that position at that time,
// resulting in only one `div` being rendered at a time but with a different key on it if the direction changes,
// so the input element passed as children gets reset when the direction changes
function IResetChildrenOnWritingDirectionChange1({ lang = "en", children }: { lang?: "en" | "fr" | "ar"; children: React.ReactNode }) {
  return (
    <div>
      {lang === "en" && <div key="ltr">{children}</div>}
      {lang === "fr" && <div key="ltr">{children}</div>}
      {lang === "ar" && <div key="rtl">{children}</div>}
    </div>
  );
}

// Another way to achieve the same effect as above
function IResetChildrenOnWritingDirectionChange2({ lang = "en", children }: { lang?: "en" | "fr" | "ar"; children: React.ReactNode }) {
  if (lang === "ar") return <div key="rtl">{children}</div>;
  return <div key="ltr">{children}</div>;
}

[bschlenk]

I'm not sure the comment above IResetChildrenOnWritingDirectionChange1 is true. What react sees in this case is actually:

return (
  <div>
    <div key="ltr">{children}</div>
    {false}
    {false}
  </div>
)

React will remount the div regardless of the key here, since this is going to be based on the slots that are filled in the jsx, not the final rendered html.

[bschlenk]

Here's a codepen example. Notice that the ref is called with null every time you change the select, regardless of the key. If it worked the way you described, then the ref wouldn't get called when switching between "First" and "Second".

[bschlenk]

Sorry I didn't read far enough ahead, I think your IResetChildrenOnWritingDirectionChange2 is a good counterpoint. It seems like this would be possible to detect though, if you're okay with only being able to catch it only when the different keys are within the same function.

[Rel1cx]

React will remount the div regardless of the key here,

The div is not important here, i mean

they can still correctly reset (or preserve) the subtree (below the children of this div) when needed

And the key here makes this behavior perform exactly the same in these two examples, regardless of whether the div itself is re-mounted or not.

I am sorry that the comment above IResetChildrenOnWritingDirectionChange1 gave a poor explanation for this, but you can test it here: https://codepen.io/undefiend/pen/zxBBwBd?editors=1010.

Type some text into the input boxes in both examples. When you click the button below to toggle the language between the first two langs, the <input> elements in neither example are reset.

When you switch to the third lang, the input elements in both examples are reset, and the text inside them is cleared. If you type new text and switch from the third lang back to either of the first two, both input elements will be reset again.

return (
  <div>
    <div key="ltr">{children}</div>
    {false}
    {false}
  </div>
)

Here, the false is also irrelevant for the demonstrated usage of using a key to control the preservation and reset of inputs in the subtree. You can directly replace all div and false in 1 with <React.Fragment> to create a 1a; the actual effect will remain the same:

function IResetChildrenOnWritingDirectionChange1a({ lang = "en", children }: { lang?: "en" | "fr" | "ar"; children: React.ReactNode }) {
  return (
    <>
      {lang === "en" ? <React.Fragment key="ltr">{children}</React.Fragment> : <></>}
      {lang === "fr" ? <React.Fragment key="ltr">{children}</React.Fragment> : <></>}
      {lang === "ar" ? <React.Fragment key="rtl">{children}</React.Fragment> : <></>}
    </>
  );
}

[Rel1cx]

It seems like this would be possible to detect though

Yes, but I tend to think that rules shouldn't be too restrictive about some potential use cases that haven't been properly explored yet. It might sound counterintuitive coming from a linter maker, but banning these potential uses from the start would stifle community innovation, especially in the React community.

[Rel1cx]

Just got back from #1381, it reminded me that for scenarios like <div {...props} key="1" /> and <div key="1" {...props} />, we also cannot report an unnecessary key.

[Rel1cx]

Actually, even this should not be reported as unnecessary:

<Menu key="one">

Because the Menu component might look like this inside:

function Menu({ key }: { key: string }) { /* do something with the key */ }

In React 19 and later with the new JSX transform, key and ref are just regular props. They can be defined and used by the user. Reporting them as unnecessary might cause them to make changes that create problems which are challenging to detect.

[Rel1cx]

We now have sufficient reasons to not to enable this rule by default too.

BTW, for those interested in what happened in IResetChildrenOnWritingDirectionChange1 and how React chooses whether to reuse or replace the <input/> elements in the example during the reconciliation process, I've created a playground link on ddir.

[bschlenk]

Sorry for commenting on a closed issue. Not sure if this changes anything, but with the new JSX transform specifically, I don't think key is ever passed down to or readable by children. Even if it gets spread in through props: https://codepen.io/bschlenk/pen/XJKNWeq?editors=1011

[Rel1cx]

Sorry for commenting on a closed issue. Not sure if this changes anything, but with the new JSX transform specifically, I don't think key is ever passed down to or readable by children. Even if it gets spread in through props: https://codepen.io/bschlenk/pen/XJKNWeq?editors=1011

You're right! The child component isn't receiving the key prop, I made a mistake on that. Thank you so much for continuing to follow up on this issue even though it's already closed.