Skip to content

Feature request: Allow index types to be subtypes of string #13285

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

Closed
danvk opened this issue Jan 4, 2017 · 8 comments
Closed

Feature request: Allow index types to be subtypes of string #13285

danvk opened this issue Jan 4, 2017 · 8 comments
Assignees
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue

Comments

@danvk
Copy link
Contributor

danvk commented Jan 4, 2017

See this example from a medium post on mapped types.

I have a function updateIds which runs some subset of properties on an object through a mapping:

> updateIds({id: 'old', other: 3},
            ['id'],
            {'old': 'new'})
{id: 'new', other: 3}

> updateIds({from: 'A', to: 'B', time: 180},
            ['from', 'to'],
            {'A': 'A1', 'B': 'B1'})
{from: 'A1', to: 'B1', time: 180}

This function is simple to implement but not entirely straightforward to type. Here's a version using keyof:

function updateIds<T>(
  obj: T,
  idFields: (keyof T)[],
  idMapping: {[oldId: string]: string}
): T {
  for (const idField of idFields) {
    const newId = idMapping[obj[idField] as any];  // <--
    if (newId) {
      obj[idField] = newId as any;  // <--
    }
  }
  return obj;
}

The problem here is that obj[idField] has a type of T[keyof T] whereas it really needs to have a type of string. I'd like the type system to require that T[k] = string for all the fields passed in the array.

I don't believe there's any way to do this directly (please tell me if I'm wrong!)

One thought was to use a second type parameter for the property list:

function updateIds<K extends string, T extends {[k: K]: string}>(
  obj: T,
  idFields: K[],
  idMapping: {[oldId: string]: string}
): T {}

But this gives the error An index signature parameter type must be 'string' or 'number'. If the index signature parameter could instead be a subtype of string (e.g. "from" | "to") then I think this would work.

TypeScript Version: 2.1.4

Code

updateIds({id: /not a string/}, 'id', {'old': 'new'})

Expected behavior:

I'd like this to be rejected since the id field doesn't have a string type.

Actual behavior:

I don't have a way to enforce this through the type system.

@mhegazy
Copy link
Contributor

mhegazy commented Jan 4, 2017

The type system can not guarantee that indexing into T with K will result in a string. the function as written allow any T.

If i understand correctly you really do not need T, what you need is a Record<K, string>, i.e.:

function updateIds<K extends string>(
    obj: Record<K, string>,
    idFields: K[],
    idMapping: { [oldId: string]: string }
): Record<K, string> {
    for (const idField of idFields) {
        const newId = idMapping[obj[idField]]; // string
        if (newId) {
            obj[idField] = newId; // string
        }
    }
    return obj;
}


updateIds({ id: /not a string/ }, ['id'], { 'old': 'new' }) // Error

updateIds({id: "strign", bar: "string" }, ['id', "bar"], {'old': 'new'}). // OK

@danvk
Copy link
Contributor Author

danvk commented Jan 4, 2017

That almost works. But it constrains all the values of the object to be strings, which I don't want to do. Only the idFields need to have string values.

updateIds({id: 'old', b: 123}, 'id', {'old': 'new'})
[ts]
Argument of type '{ id: string; b: number; }' is not assignable to parameter of type 'Record<string, string>'.
  Property 'b' is incompatible with index signature.
    Type 'number' is not assignable to type 'string'.
(property) id: string

@mhegazy
Copy link
Contributor

mhegazy commented Jan 4, 2017

I see. you really need the T then. here is a simplified repro:

function updateIds<T extends { [x: string]: string }, K extends keyof T>(
    obj: T,
    key: K,
    stringMap: { [oldId: string]: string }
) {
    var x = obj[key];
    stringMap[x]; // Should be OK.
}

@mhegazy mhegazy added the Bug A bug in TypeScript label Jan 4, 2017
@mhegazy mhegazy added this to the TypeScript 2.2 milestone Jan 4, 2017
@danvk
Copy link
Contributor Author

danvk commented Jan 4, 2017

@mhegazy is that right? is K extends keyof T constrained to be a key that maps to a string in your example?

@ahejlsberg ahejlsberg added the Fixed A PR has been merged for this issue label Jan 10, 2017
@ahejlsberg
Copy link
Member

@danvk With #13338 the following compiles with no errors:

function updateIds<T extends Record<K, string>, K extends string>(
    obj: T,
    idFields: K[],
    idMapping: { [oldId: string]: string }
): Record<K, string> {
    for (const idField of idFields) {
        const newId = idMapping[obj[idField]];
        if (newId) {
            obj[idField] = newId;
        }
    }
    return obj;
}

@danvk
Copy link
Contributor Author

danvk commented Jan 10, 2017

Thanks, @ahejlsberg. Keep up the great work!

@danvk danvk closed this as completed Jan 10, 2017
@zpdDG4gta8XKpMCd
Copy link

zpdDG4gta8XKpMCd commented Jan 10, 2017

wouldn't it be better for K extends string be allowed in [key: K]: ...?

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
@ahejlsberg
Copy link
Member

ahejlsberg commented Dec 11, 2018

@danvk In #28965 we are tightening up some rules related to your example. I suggest you change it to the following, which is more correct (see comments in #28965 for why the type annotation is necessary):

function updateIds<T extends Record<K, string>, K extends string>(
    obj: T,
    idFields: K[],
    idMapping: Partial<Record<T[K], T[K]>>
): Record<K, string> {
    for (const idField of idFields) {
        const newId: T[K] | undefined = idMapping[obj[idField]];
        if (newId) {
            obj[idField] = newId;
        }
    }
    return obj;
}

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

4 participants