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.

Sign up for GitHub

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

fix(propEq): improve propEq typings #72

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

fix(propEq): improve propEq typings #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion test/anyPass.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ expectType<boolean>(
})
);

expectError(
expectType<boolean>(
isVampire({
age: 21,
garlic_allergy: true,
Expand Down
35 changes: 24 additions & 11 deletions test/propEq.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { expectError, expectType } from 'tsd';

import { propEq } from '../es';
import {__, propEq} from '../es';

type Obj = {
union: 'foo' | 'bar';
Expand All @@ -12,28 +12,41 @@ type Obj = {

// propEq(val, name, obj)
expectType<boolean>(propEq('foo', 'union', {} as Obj));
// non-union string fails
expectError(propEq('nope', 'union', {} as Obj));
// completely different type fails
expectError(propEq(2, 'union', {} as Obj));
// propEq doesn't create unnecessary values constraint for object
expectType<boolean>(propEq(1, 'union', {} as Obj));
// unknown field names fails
expectError(propEq('foo', 'unknown', {} as Obj));

// propEq(val)(name)(obj)
expectType<boolean>(propEq('foo')('union')({} as Obj));
// 'nope' is inferred as 'string' here.
expectType<boolean>(propEq('nope')('union')({} as Obj));
// completely different type fails
expectError(propEq(2)('union')({} as Obj));
// unknown field names fails
expectError(propEq('foo')('unknown')({} as Obj));

// propEq(val)(name), obj)
expectType<boolean>(propEq('foo')('union', {} as Obj));
// 'nope' is inferred as 'string' here.
expectType<boolean>(propEq('nope')('union', {} as Obj));
// completely different type fails
expectError(propEq(2)('union', {} as Obj));
// unknown field names fails
expectError(propEq('foo')('unknown', {} as Obj));

// propEq(val, name)(obj)
expectType<boolean>(propEq('foo', 'union')({} as Obj));
// 'nope' is inferred as 'string' here.
expectType<boolean>(propEq('nope', 'union')({} as Obj));
// completely different type fails
expectError(propEq(2, 'union')({} as Obj));
// unknown field names fails
expectError(propEq('foo', 'unknown')({} as Obj));

// propEq(__, name, obj)(val)
expectType<boolean>(propEq(__, 'union', {} as Obj)('foo'));
// propEq(val, __, obj)(val)
expectType<boolean>(propEq('foo', __, {} as Obj)('union'));
// propEq(__, __, obj)(val, name)
expectType<boolean>(propEq(__, __, {} as Obj)('foo', 'union'));
// propEq(__, __, obj)(val)(name)
expectType<boolean>(propEq(__, __, {} as Obj)('foo')('union'));

expectError(propEq('foo', __, {} as Obj)('unknown'));
expectError(propEq(__, __, {} as Obj)('foo', 'unknown'));
expectError(propEq(__, __, {} as Obj)('foo')('unknown'));
35 changes: 30 additions & 5 deletions types/propEq.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,31 @@
export function propEq<T>(val: T): {
<K extends PropertyKey>(name: K): (obj: Record<K, T>) => boolean;
<K extends PropertyKey>(name: K, obj: Record<K, T>): boolean;
import { Placeholder } from 'ramda';

export function propEq(__: Placeholder): never;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me, however I'm not sure how necessary it it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be a bit extra, but it can save from misusing the placeholder

export function propEq<V>(val: V): {
<K extends PropertyKey>(name: K): (obj: Record<K, any>) => boolean;
<U extends Record<PropertyKey, any>>(__: Placeholder, obj: U): (name: keyof U) => boolean;
<K extends keyof U, U extends Record<PropertyKey, any>>(name: K, obj: U): boolean;
};
export function propEq<T, K extends PropertyKey>(val: T, name: K): (obj: Record<K, T>) => boolean;
export function propEq<K extends keyof U, U>(val: U[K], name: K, obj: U): boolean;
export function propEq<K extends PropertyKey>(__: Placeholder, name: K): {
(__: Placeholder, obj: Record<K, any>): <V>(val: V) => boolean;
<V>(val: V, obj: Record<K, any>): boolean
<V>(val: V): (obj: Record<K, any>) => boolean
Comment on lines +10 to +12
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Overload order is important.
  • WidenLiterals<> should be used in place of any
Suggested change
(__: Placeholder, obj: Record<K, any>): <V>(val: V) => boolean;
<V>(val: V, obj: Record<K, any>): boolean
<V>(val: V): (obj: Record<K, any>) => boolean
<V>(val: V): (obj: Record<K, WidenLiterals<V>>) => boolean
<U extends Record<K, any>>(__: Placeholder, obj: U): <V extends WidenLiterals<U[K]>>(val: V) => boolean;
<V>(val: V, obj: Record<K, WidenLiterals<V>>): boolean

};
export function propEq<V, K extends PropertyKey>(val: V, name: K): (obj: Record<K, any>) => boolean;


export function propEq<U extends Record<any, any>>(__: Placeholder, ___: Placeholder, obj: U): {
(__: Placeholder, name: keyof U): {
(__: Placeholder): never;
<V>(val: V): boolean;
}
<V>(val: V, name: keyof U): boolean;
(__: Placeholder): never;
<V>(val: V): (name: keyof U) => boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per my main comment, I don't think (val: Placeholder): never is needed, since val should be U[K] here, and thus can never be Placeholder

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to demonstrate in my main comment that this kind of typing is more likely to create more unnecessary obstacles, rather than help developers catch errors in the early stage

};
Comment on lines +17 to +25
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overload order + WidenLiterals<>

Suggested change
export function propEq<U extends Record<any, any>>(__: Placeholder, ___: Placeholder, obj: U): {
(__: Placeholder, name: keyof U): {
(__: Placeholder): never;
<V>(val: V): boolean;
}
<V>(val: V, name: keyof U): boolean;
(__: Placeholder): never;
<V>(val: V): (name: keyof U) => boolean;
};
export function propEq<U extends Record<PropertyKey, any>>(__: Placeholder, ___: Placeholder, obj: U): {
<V>(val: V): (name: keyof U) => boolean;
<K extends keyof U>(__: Placeholder, name: K): (val: WidenLiterals<U[K]>) => boolean;
<K extends keyof U>(val: WidenLiterals<U[K]>, name: K): boolean;
};

export function propEq<K extends PropertyKey>(__: Placeholder, name: K, obj: Record<K, any>): {
(__: Placeholder): never;
<V>(val: V): boolean;
Comment on lines +27 to +28
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from my personal opinion about the usefulness of (__: Placeholder): never being present throughout, there is a technical reason why as well

It has to do with overloads and generics, see here: #54

So there is a need to remove all instances of (__: Placeholder): never do need to be removed

See it in action against your changes here: https://tsplay.dev/W4an4w

Comment on lines +26 to +28
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export function propEq<K extends PropertyKey>(__: Placeholder, name: K, obj: Record<K, any>): {
(__: Placeholder): never;
<V>(val: V): boolean;
export function propEq<K extends keyof U, U extends Record<PropertyKey, any>>(__: Placeholder, name: K, obj: U): (val: WidenLiterals<U[K]>) => boolean;

};
export function propEq<V, U extends Record<any, any>>(val: V, __: Placeholder, obj: U): (name: keyof U) => boolean;
export function propEq<V, U extends Record<any, any>>(val: V, name: keyof U, obj: U): boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V needs at least some constraint, we can use WidenLiterals<> here as well

Suggested change
export function propEq<V, U extends Record<any, any>>(val: V, name: keyof U, obj: U): boolean;
export function propEq<K extends keyof U, U extends Record<PropertyKey, any>>(val: WidenLiterals<U[K]>, name: K, obj: U): boolean;