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

propEq type definition does not guarantees type safety #476

Closed
peterreisz opened this issue Jun 3, 2020 · 5 comments
Closed

propEq type definition does not guarantees type safety #476

peterreisz opened this issue Jun 3, 2020 · 5 comments

Comments

@peterreisz
Copy link

The current definition accepts any as the last parameter

export function propEq<T>(propToFind: string | number, valueToMatch: T, obj: any): boolean;
export function propEq<T>(propToFind: string | number, valueToMatch: T): (obj: any) => boolean;
export function propEq(propToFind: string | number): {
  <T>(valueToMatch: T, obj: any): boolean;
  <T>(valueToMatch: T): (obj: any) => boolean;
};

The type safe solution would be something like this:

export function propEq<T, K extends keyof T>(propToFind: K, valueToMatch: T[K], obj: T): boolean;
export function propEq<T, K extends keyof T>(propToFind: K, valueToMatch: T[K]): (obj: T) => boolean;
export function propEq<T, K extends keyof T>(propToFind: K): {
   (valueToMatch: T[K], obj: T): boolean;
   (valueToMatch: T[K]): (obj: T) => boolean;
};
selfrefactor added a commit that referenced this issue Jun 3, 2020
@selfrefactor
Copy link
Owner

Thank you @peterreisz for pointing this out.

I copied the definitions from @types/ramda and I created a dtslint test to confirm the change. You can see it in the referenced commit.

@selfrefactor
Copy link
Owner

Released with 5.6.1

@peterreisz
Copy link
Author

@selfrefactor there is a problem with the Record solution when using optional properties.
When the type has optional property you cannot write code which compiles.
The built-in Record type constructs a new type, but in that type the prop will be required, which will not match with the given type.

interface MyType {
    optional?: string | number;
}

function propEqWithRecord<K extends string | number, V>
  (propToFind: K, valueToMatch: V, obj: Record<K, V>): boolean {
    return false;
}

function propEqWithoutRecord<K extends keyof T, T>
  (propToFind: K, valueToMatch: T[K], obj: T): boolean {
    return false;
}

const myObject: MyType = {};
const valueToFind = '1111';
const optionalValueToFind: string | number | undefined = '1111';

// These will not compile
propEqWithRecord('optional', valueToFind, myObject);
propEqWithRecord('optional', optionalValueToFind, myObject);

// These will compile
propEqWithoutRecord('optional', valueToFind, myObject);
propEqWithoutRecord('optional', optionalValueToFind, myObject);

@selfrefactor
Copy link
Owner

@peterreisz I will run dtslint test and most probably I will apply your suggestion.

@selfrefactor
Copy link
Owner

Released with 5.6.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants