Rule proposal: Prefer array.at(...) over array[...] as T | undefined #8979
Comments
|
big +1 ❤️
|
|
We didn't used to use this cos we supported old nodejs versions! |
|
Some thoughts: Would it make sense to generalize it to prefer Should this also fail too? declare const values: string[];
declare const index: number;
const a: string | undefined = values[index];Also regarding other contextual types of declare const values: string[];
declare const index: number;
// explicit return type annotation
function foo(): string | undefined {
return values[index];
}
// function argument
bar(values[index]);
function bar(s: string | undefined) {} |
|
Couldn't there be some conflict with Since both options are not related a priori, someone would want both so that: const item = array[0]could be fixed either with: const [item] = arrayor: const item = array.at(0)Maybe could it be handled through an |
|
@OlivierZal no because I would expect that the eventual implementation should flag the destructuring code and suggest using |
I don't think it would, in the same sence that |
|
Heh okay. I only quickly glanced at the top and missed that it requires annotation. |
|
noUncheckedIndexedAccess by itself is a very contentious option since you are essentially shutting out all C-style for loops. This lint rule, without the contextual type, is marginally better than forcing people to turn on NUIA: it only forces you to use a less optimized alternative and creates a (IMO unnecessary) distinction between arrays and index signatures. |
|
If we're just targetting Quickly searching via sourcegraph returns LESS THAN 600 RESULTS. With that low volume IMO it distinctly fails the final check:
Personally I would raise a motion to close this rule proposal as too niche. |
|
You are searching for indexing with a constant, but most indexing happens with a variable. Adding that gives you 1.7k—idk if that's better. |
|
Frame it this way: If we turned on this rule across the entirety of the public GitHub TypeScript code -- likely billions of lines of code -- there would be just 1.7k reports for this rule. I ran the grep (with Seems like it's niche enough that the cost:value proposition just isn't there. |
|
eh, yeah, i'm ok with this being closed |
|
@JoshuaKGoldberg @Josh-Cena thoughts? |
|
Yeah I'd prefer it to exist but don't feel passionately. The lack of applicable code or public excitement tampered my enthusiasm. |
|
No strong feelings from me |
|
rip |
Uh oh!
There was an error while loading. Please reload this page.
Before You File a Proposal Please Confirm You Have Done The Following...
My proposal is suitable for this project
Description
Array index lookups aren't type-safe without the often-overly-strict
noUncheckedIndexedAccess: they give backT, notT | undefined. A lot of TS code that predatesArray.prototype.at()had to look like[index] as T | undefinedinstead of.at(index). Example in our repo:typescript-eslint/packages/eslint-plugin/src/rules/prefer-optional-chain-utils/analyzeChain.ts
Line 97 in eef257b
Now that we have
.at(), I think it makes sense to enforce that: if an array index lookup uses a type assertion to add| undefined, it should use.atinstead.Fail Cases
Pass Cases
Additional Info
This is vaguely familiar to the idea of
@typescript-eslint/no-unnecessary-type-assertion. But it includes a runtime change.The text was updated successfully, but these errors were encountered: