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

Bug: [consistent-indexed-object-style] shouldn't suggest Record when extending an another type #9068

Open
4 tasks done
ChrisMBarr opened this issue May 9, 2024 · 7 comments
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look

Comments

@ChrisMBarr
Copy link

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.4.3&fileType=.ts&code=JYOwLgpgTgZghgYwgAgLIE8DCBXAzmAewFsAVAC1AHNkBvAWAChlkYCCAuZfKKgbkeYAjOFE7c%2BA5MIBenek2bIAjtgAeYsDxCUA2gF1%2BCgL6MTDRqEixEKAJIgAJhFVxBAGwjkqAOQK3cuNgQuMjOkI4hGDj4xF7atJI6ANYQ6BpalHqc2CBJIAQA7iCGzARgZNCcINhEgtCGZhbg0PBIyPZOLu6eFNoA6sDl-oHBoarhDpFYeISkvdTyzMmp6VRZyDl5hcWmQA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6MgeyeUuX0Ra1mAE0QAPRMNocARgCtEZOn0JJ0URNGgdokcGAC%2BIA0A&tsconfig=&tokens=false

Repro Code

interface MyCustomThing {
  foo: string;
  bar: string;
  baz: {
    qux: string[];
  }
}

//This is OK since I add the "other" property, it does not suggest a Record
interface IndexableThingNoIssues extends MyCustomThing {
  [key: string]: unknown;
  other: number;
}

//An error is thrown here, but I don't think it should since other properties do exist due to it extending
interface IndexableThingWithIssues extends MyCustomThing {
  [key: string]: unknown;
}

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/consistent-indexed-object-style": "error"
  },
};

tsconfig

No response

Expected Result

Since I am extending an object I would expect no errors.

Actual Result

Error with "A record is preferred over an index signature." even though it's extending an existing type. Replacing this with a Record is not the equivalent code

Additional Info

No response

@ChrisMBarr ChrisMBarr added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels May 9, 2024
@kirkwaiblinger
Copy link
Member

kirkwaiblinger commented May 9, 2024

Hmmm, the comments in this PR tell an interesting story for this case: #3009

This behavior seems to be intentional...
a) autofix is not available when there is a (nonempty) extends list (because it... can't really be fixed?)
b) the rule won't report when there's more than one property in the interface, regardless of the extends list.

The case you bring up falls where those two entirely separate behaviors interact

@ChrisMBarr
Copy link
Author

I'm surprised this has never come up before! Sometimes I just need to be able to access properties by index. This particular case is for some custom error logging where I want to grab all properties off of an incoming error, but exclude a few by name. TS complained at me for not having an index signature, so I added this. Here's my real world use case for the type I ran into this on

export interface IIndexableAxiosError extends AxiosError {
  [key: string]: unknown;
}

@bradzacher
Copy link
Member

I'm surprised this has never come up before! Sometimes I just need to be able to access properties by index.

It's generally very, very rare that you want to add an unbounded indexer to an existing type!
It introduces a fair bit of unsafety (eg you can easily pass an invalid key).
Most people don't want to index an object with known keys using a raw string typed variable.

Or a safer way to do things would be eg

declare const key: string;
declare const base: AxiosError;
if (base.hasOwnProperty(key)) {
  base[key as keyof typeof base];
}

I don't think there's a really compelling, broadly-applicable reason to allow this within the rule.
I think leaving it as an error and having the user add a disable comment explaining why they're opting in to an indexer is a better outcome than just ignoring it and making it look like a false negative.

@ChrisMBarr
Copy link
Author

ok, fair enough. I've updated my code to grab the value using let val: unknown = error[key as keyof AxiosError]; which does feel a bit safer to me than what I was doing before. So thanks for that.

However, given my original code it still feels kinda wrong to recommend that an extended interface with an added index be recommended to use as as Record instead. That recommendation doesn't feel right to me since it can't really be done. Perhaps for this unique situation it could have a different error message?

@ChrisMBarr
Copy link
Author

ChrisMBarr commented May 14, 2024

ok, so I've actually run into this issue yet again in a slightly different way. I have a large "base" interface and then I make several smaller and specific interfaces with Pick or Omit` for different use cases. I need them to be indexable.

Here's a playground link for everything below.

I could go this route:

interface ILargeBaseA {
  foo: string;
  bar: number;
  baz: boolean
  qux: string[];
  zop: null | string;
}

interface IStringsOnlyA extends Pick<ILargeBaseA, 'foo' | 'qux' | 'zop'> {
  [key: string]: string | string[] | null;
}

function getValA(obj: IStringsOnlyA, key: string) {
  return obj[key];
}

which works great and the return type of getValA is correctly inferred as string | string[] | null but this rule again complains that it would prefer it to be a Record.

Ok, I'll lean into that and move the indexing to the base class and use a Record as it wants

interface ILargeBaseB {
  [key: string]: string | string[] | boolean | number | null;
  foo: string;
  bar: number;
  baz: boolean
  qux: string[];
  zop: null | string;
}

type IStringsOnlyB = Pick<ILargeBaseB, 'foo' | 'qux' | 'zop'>;

function getValB(obj: IStringsOnlyB, key: string) {
  return obj[key];
}

ESLint no longer complains, but now I lose my index signature it it just infers an any type from that function. Using "strict":true this will give a compilation error

Element implicitly has an 'any' type because expression of type 'string' can't be used to index type 'IStringsOnlyB'.
  No index signature with a parameter of type 'string' was found on type 'IStringsOnlyB'.

as far as I can tell, my only workaround here is to disable this rule since it just seems flat out wrong my my use case here.

a possible workaround I might be able to do is use keyof like this, which accomplishes it all, but it may not always be feasible in certain situations

interface ILargeBaseC {
  [key: string]: string | string[] | boolean | number | null;
  foo: string;
  bar: number;
  baz: boolean
  qux: string[];
  zop: null | string;
}

type IStringsOnlyC = Pick<ILargeBaseC, 'foo' | 'qux' | 'zop'>;

function getValC(obj: IStringsOnlyC, key: keyof IStringsOnlyC) {
  return obj[key];
}

@bradzacher
Copy link
Member

Why not something like

type IStringsOnlyC = Pick<ILargeBaseC, 'foo' | 'qux' | 'zop'> & Record<string, string | string[] | null>;

Or more generally

type IndexableThing = T & Record<string, values of T>;

What's wrong with an intersection here?

@ChrisMBarr
Copy link
Author

I do not use intersection types often, thanks for the reminder. ok, good call there.

but again, perhaps the rule could make more intelligent recommendations/fixes for cases like these?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look
Projects
None yet
Development

No branches or pull requests

3 participants