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

Enums, string/number types of interfaces/classes cannot be used as index signatures #37448

Closed
jpike88 opened this issue Mar 18, 2020 · 23 comments
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@jpike88
Copy link

jpike88 commented Mar 18, 2020

Due to unresponsiveness and a highly unsatisfactory outcome on issue #1778, I want to raise a fresh issue so I can get a clear and final outcome. I don't mind if it's another no (actually I would because it makes zero sense), but if it's a no, I would like something that wasn't provided in the referenced issue: a CLEAR solution to this problem.

So let's recap with some basics:

  • There are two types of enum: number, or string.
  • A type can be a string or number, or contain a property that is a string or number
  • There are two types of supported index signatures: string and number

It naturally follows that index signatures should be able to be aliased, purely by observing the above facts alone.

But let's step into a practical scenario.

It's common to make the equivalent of a hashmap or lookup map, whether it be for the developer's convenience, or for performance reasons. Either way, it's almost always the case in my experience that the indexes of these hash maps correspond to a type expressed elsewhere in the codebase, and aren't just an arbitrary/random index value.

e.g.
public templateElementHashMap: { [s: string]: TemplateElement } = {};

This shows an incomplete context, as the string type is always going to be the hash property in the class below:

export class TemplateElement {
     public hash?: string;
}

Therefore, the truest expression would be this:

public templateElementHashMap: { [s: TemplateElement['hash']]: TemplateElement } = {};

This provides full context to the developer, quick travel to the type of interest, and as long as TemplateElement['hash'] is string or number, the above statement is 100% correct, and does not break any rules regarding type consistency. It's a win/win, and clearly a large improvement.

Instead right now, I have many lines where the index signature is just a string property, and it's become a pain to remember which hash map relates to which data I'm using it for. And quite often, I'm wasting time poking around trying to remember which index derives from which type.

So I'm going to ask again: what is the solution? To me there are only two:

  • Make a comment after each one (zero type security, comments can be out of date, it's ugly, why bother using TypeScript if I'm trying to compensate for its shortcomings with comments everywhere)
  • Get on Github and start making noise about it

I've poured a fair amount of my time providing feedback and input for TypeScript since its early days, and only ask this is treated with the promptness and attention that I have received in the past, and not closed prematurely like the other issue.

So please can I receive a clear and concise workaround, or (preferably) the acknowledgement that something actually is inadequate with the design around this and I'm not just going insane, along with the many others who have expressed the same problem.

@MartinJohns
Copy link
Contributor

This provides full context to the developer, quick travel to the type of interest, and as long as TemplateElement['hash'] is string or number, the above statement is 100% correct, and does not break any rules regarding type consistency. It's a win/win, and clearly a large improvement.

Just pointing out that in your example the type is not string or number, in your example the type is string | undefined.

@jpike88
Copy link
Author

jpike88 commented Mar 18, 2020

Sorry I was being rough... you’re correct

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Mar 18, 2020
@jpike88
Copy link
Author

jpike88 commented Mar 19, 2020

@RyanCavanaugh I understand the point you're making.

I already been commenting each line, and it's making more problems than it solves. As trivial as that sounds, having a live type reference with fast travel greatly enhances readability, it also will error if that property no longer exists/gets changed, which would make refactoring much safer.

But it would be a shame to not allow SOME way of allowing this to take place. So the solution may be some sort of alternative syntax to indicate that type safety isn't being provided, you're just casting in the index to show this? Isn't this really just a question of syntax if my original suggestion hits the wall you're describing?

Some ideas:
public templateElementHashMap: { [s: string as TemplateElement['hash']]: TemplateElement } = {};

public templateElementHashMap: { [s: string indexedAs TemplateElement['hash']]: TemplateElement } = {};

public templateElementHashMap: { [s: string]: TemplateElement } indexedAs TemplateElement['hash'] = {};

@Vinnl
Copy link

Vinnl commented Mar 19, 2020

(Hmm, I'm seeing a comment in my email that is not present here in GitHub... I'm responding to that.)

Although I'd like to see type aliases being allowed as index signatures, it's obviously up to the TypeScript's team discretion, so please don't take this comment as demanding that the feature be added.

However, there's something I don't understand yet, that someone might be willing to clear up.

If we add some ability to indirectly reference string through some other type construction, it's creating the appearance that it's an enforcement mechanism, because otherwise why would it be possible?

The argument against type aliases as index signatures, as I understand it, comes down to "it looks like we're enforcing a type there, but we're not - any string is valid, so allowing type aliases as index signatures would be misleading". I get that point.

However, what I don't get, is: is that not an argument against having type aliases in general?

For example:

type SomeType = string;

function someFunction (arg: SomeType) { /* do something */ }

const someValueThatIsNotSomeType: string = 'hiya';
someFunction(someValueThatIsNotSomeType); // valid

Does the argument not hold there as well? We pretend that someFunction only accepts SomeType, but it will take any string and typecheck just fine?

@jlennox
Copy link

jlennox commented Mar 19, 2020

@Vinnl This is the exact point I made here #1778 (comment)

The official response to which was

All the prior input we've provided is still true

But that point has never been addressed and describes exactly what is wanted here. This addition would infer no safety beyond exactly what the language already does. The argument that it increases confusion does not hold water because this addresses a language inconsistency.

I've also been wondering who the others in the plural "we've" are and why they have not spoken.

This is marked as Awaiting More Feedback (which is described as: "This means we'd like to hear from more people who would be helped by this feature") but there's 5 years of community feedback in #1778 already.

@jpike88
Copy link
Author

jpike88 commented Mar 25, 2020

This issue has gone the same route as the last one. Solid points are being made and it goes nowhere.

@RyanCavanaugh is the official Typescript team recommendation for this issue to just add comments everywhere?

@jpike88
Copy link
Author

jpike88 commented Mar 26, 2020

@RyanCavanaugh I also second removing the 'Awaiting more feedback', I'm not sure how much more feedback you need for the point to be driven home. In issue #1778 and in this one there are valid points being made, and the response has been to just ignore them and abandon the issue.

Is that what's happening here? Again?

@orta
Copy link
Contributor

orta commented Mar 26, 2020

I've also been wondering who the others in the plural "we've" are and why they have not spoken.

We, the compiler team, get hundreds of comments every day on this repos issues. It's often much easier to let one person respond when they are closer to the domain. We already have to pick our battles in these issues to actually get work done, and mix that with people being less available from COVID19 and it's difficult to feel any progress on the language. For example, I used about 45m on just writing this tiny 3 paragraph reply.

I get that this issue is something close to your heart, but the team has commented on this issue multiple times with the same consistent opinion through the years in the original thread.

In my own OSS, I usually recommend that if someone is this passionate about a feature they should look at implementing it to try and better understand the constraints of the system. Maybe try give this a shot and have a better chance of coming at it from. language maintainers perspective too?

@jpike88
Copy link
Author

jpike88 commented Mar 26, 2020

I appreciate that.

I don’t have the skill for the time to contribute to this in a competent way, I wish I did. But this feels like a repeat of what occurred last issue. And it’s a frustrating experience because I don’t necessarily want an improvement to TypeScript on my command done overnight, but a concise and clear answer that directly addresses concerns being raised and acknowledges there’s an actual problem here.

I’m guessing at this point that the ‘official’ stance is that I have to make comments. I’ve already been doing that and will live with it... thanks for your time, I do appreciate it.

@jamietre
Copy link

jamietre commented Jun 22, 2020

We use interfaces derived from String extensively to nominally type strings that should not be interchangeable. This is very convenient and within the API boundary of our software we never have to do type conversions and have complete safety for use of these string types... EXCEPT as object keys. For example:

export interface ReportName extends String {
  _nominal: "ReportName";
}

export interface ReportId extends String {
  _nominal: "ReportId";
}

Now we can write functions that consume these types, instead of plain strings, and we get a compile error if we mix them up. It would be amazing if I could say

type Reports = {
   [key: ReportId]: Report
}

... but I cannot. The workarounds are to convert these types back and forth to strings whenver using indexes, or to use Map objects instead. The latter is not really a great option, since we'd be doing a lot of conversion of objects to maps, and you just get a lot of other stuff for free with objects in TypeScript. So we just end up with clumsy, non-typesafe casts when using these interfaces as keys.

Anything that could add support for using any String derived interface as an index key would be amazing.

@Samox
Copy link

Samox commented Jun 22, 2020

Same problem here, I wish I could index from branded types (which are very useful 😇) but I'm not sure how to do it properly. (Not sure the problem belongs to this issue).
image

@KaeruCT
Copy link

KaeruCT commented Jul 30, 2020

As a workaround I've been using a Record instead of an object type. Hopefully this is helpful to someone else.

Using @jamietre's case as an example:

type Reports = {
   [key: ReportId]: Report
}

can be

type Reports = Record<ReportId, Report>;

@alecf
Copy link

alecf commented Oct 6, 2020

The Record solution is a great way to declare such a mapping - it's a great first step. unfortunately it leads to implicit anys when you actually look something up in the record - see #40892

@RobertAKARobin
Copy link

Just jumping on the bandwagon in support of this feature. It would add a great deal of clarity.

@shellscape
Copy link

Throwing in my support for this. I was attempting to use [level: Uppercase<string>]: number; today and ran into the error.

@f-o-w-l
Copy link

f-o-w-l commented Aug 4, 2021

Looking forward to this issue being resolved!

I do not understand the reasoning from #1778 that this would "create the implication that you're making different kinds of index signature". If the type aliasing was resolved and checked by the compiler, there would be no implications, it would either compile and one would be able to hover over it in an IDE to check the resolved type, or error and let one know that the type did not resolve to an allowed type.

Much thanks to @alecf for the temporary workaround for the need to add a comment explaining the type relation.

@zacksinclair
Copy link

zacksinclair commented Aug 6, 2021

In the spirit of the "Awaiting More Feedback" tag -

I came across this exact situation and was very confused / surprised to find I wasn't able to use an alias to describe the key. I now have comments in the code base explaining what could otherwise be more accurately described by a type alias. More cognitive overload for the next guy.

I think Ryan's point here (from #1778 (comment)):

We think allowing you to write type aliases here creates the implication that you're making different kinds of index signature, but you're not. It's actively confusing and doesn't create any new functionality.

is missing the key ingredient - that, for future maintainers and reviewers, it is in fact MORE confusing that we can't alias the type, when the rest of the language allows us to use aliases for virtually everything and the alternative is an extra comment linking to these issues.

Instead we have to read comments and GitHub issue threads that basically boil down to "in this specific instance we don't want to improve the typing of your objects because javascript is the underlying and you could hypothetically monkeypatch it elsewhere" (which could likely be detected in tsc, no?).

By way of example, which is more confusing to the next person to come across this code?

class User {
  id: string
}
class Post {
  id: string
}

const postToUsers: { [key: Post['id']]: User } = {}

or

class User {
  id: string
}
class Post {
  id: string
}

// key is actually Post['id'] but aliases arent allowed because reasons: https://github.com/microsoft/TypeScript/issues/37448
const postToUsers: { [key: string]: User } = {}

Furthermore downstream consumers don't know that this object's keys actually refer to something specific and are not intended to be arbitrary. That's pretty confusing.

@shellscape
Copy link

@zacksinclair exceptional rebuttal 🍺

@jlennox
Copy link

jlennox commented Aug 13, 2021

I'd be interested if @ahejlsberg has any input on the topic.

@jamesgpearce
Copy link

jamesgpearce commented Sep 5, 2021

This started working in TS 4.4:

class User {id: string = '4'}
class Post {id: string = 'hello'}
const postToUsers: {[key: Post['id']]: User} = {}

Digging deeper, the new index signature features are in the 4.4 release notes.

@jlennox
Copy link

jlennox commented Sep 6, 2021

@jamesgpearce

Thanks for pointing this out. This does solve the initial issue raised in #1778. It's unfortunate it took this 6 year road and had the responses it got.

@ahejlsberg
Copy link
Member

Indeed, this issue was fixed by #44512 which is included in TS 4.4. Also, in TS 4.4 and later it is possible to have index signatures for tagged primitive types, for example:

type Id = string & { __tag: 'id '};
type Rec = Record<Id, number>;

declare const s: string;
declare const id: Id;
declare const rec: Rec;

rec[s] = 1;   // Error
rec[id] = 1;  // Ok

@ahejlsberg ahejlsberg added Fixed A PR has been merged for this issue and removed Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature labels Sep 7, 2021
@saftipierreminiggio
Copy link

Indeed, this issue was fixed by #44512 which is included in TS 4.4. Also, in TS 4.4 and later it is possible to have index signatures for tagged primitive types, for example:

type Id = string & { __tag: 'id '};
type Rec = Record<Id, number>;

declare const s: string;
declare const id: Id;
declare const rec: Rec;

rec[s] = 1;   // Error
rec[id] = 1;  // Ok

YEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEEESSSSSSSSSSSSSSSSSSSSSSSSSS ! ♥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests