fix(propEq): improve propEq typings

[Nemo108]

• remove unnecessary constraint from propEq value: the function should receive any type values of object, no matter what parameter it received;
• add additional types to use with placeholder

[Harris-Miller]

remove unnecessary constraint from propEq value: the function should receive any type values of object, no matter what parameter it received;

I'm afraid I have to disagree with this. It is not unnecessary at all. It is important for type safety. With the current implementation, you get this level of safety

import { propEq } from 'ramda';

const doesFooEq123 = propEq(123, 'foo');
//    ^? (obj: Record<'foo', number>) => boolean

// now consider how this gives a type error
'123' === 123; // TypeError: This comparison appears to be unintentional because the types 'string' and 'number' have no overlap.

// With the current typing, you get a similar error
doesFooEq123({ foo: '123' }); // Type 'string' is not assignable to type 'number'.

That error at the end would not happen with your updated, and that's not something that we should loosen

Playground: https://tsplay.dev/wgBX9W

add additional types to use with placeholder

The added currying support with Placeholder is fine, however there are a few situation that don't make sense to me, I'll leave inline comments for those

[Harris-Miller]

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

[Nemo108]

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

[Harris-Miller]

Should the final return not be never? That would match the intention of the very first overload

[Nemo108]

Thank you for noticing. You are right, it should be never here

[Harris-Miller]

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

[Nemo108]

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

[Nemo108]

Hi! Thank you for such a detailed review.

I understand your position on typing and generally support it. But when it comes to this particular case, while using this function I often encountered problems that are created by overly strict typing, while I did not find any advantages in strict typing of the first parameter on real-world projects.

You see, most of the time you have to deal with more general types, especially when you heavily use other ramda functions. Here you can see the simplest of examples when you get false negative results on type checking.

There were more problems that I faced when working on real projects and most of the time I had to typecast as quick, dirty, and hacky solutions.

If you can come up with a better way to define the types of the first parameter that eliminates problems like this, I'll be happy, but in the meantime, I have to insist on simplifying the typing a little.

[Nemo108]

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

[Nemo108]

Thank you for noticing. You are right, it should be never here

[Nemo108]

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

[Harris-Miller]

I looked at your example and added an additional test:

type Obj = { key: 'A' | 'B' };

function doesEqKey(val: string, obj: Obj) {
    return obj.key === val;
}

typescript here gives no issue. The reason why I found is because at last one side as assignable to the other. However, do understand that val would not be assignable to obj.key

obj.key = val; // Type 'string' is not assignable to type '"A" | "B"'.

So for assoc, we would want to keep the way it is typed, same as propEq is now. But you are correct that propEq does need to be loosened a bit.

However, any is still not appropriate here, because we don't want to mix types.

function doesEqKey(val: number, obj: Obj) {
    return obj.key === val; // This comparison appears to be unintentional because the types 'string' and 'number' have no overlap 
}

We do have a solution for what you are looking for you. In types/util/tools.d.ts use WidenLiterals<T>

import { WidenLiterals } from './util/tools';

export function propEq<K extends keyof U, U>(val: WidenLiterals<U[K]>, name: K, obj: U): boolean;

What this will do is collapse down val from 'A' | 'B' to be string. This should properly mimic the behavior for both doesEqKey functions I have above. Particularly that any string can be compared to 'A' | 'B', but not number or other types that don't overlap

[Nemo108]

Thank you for the tip about WidenLiterals type. It did help me and fixed some of the false negative results that I had.

Unfortunately, there are a lot of other kinds of errors that I wasn't able to fix. I created another draft PR here, where you can find more tests and the version of propEq typings that utilizes WidenLiterals, but as you can see, most of the tests are failing. Moreover, other tests are failing too, like anyPass and allPass.

const isOld = propEq(212, 'age');
const isAllergicToGarlic = propEq(true, 'garlic_allergy');
const isAllergicToSun = propEq(true, 'sun_allergy');
const isFast = propEq(null, 'fast');
const isAfraid = propEq(undefined, 'fear');

const isVampire = anyPass([
  isOld,
  isAllergicToGarlic,
  isAllergicToSun,
  isFast,
  isAfraid
]);

expectType<boolean>(
  isVampire({} as {
    age: number,
    garlic_allergy: boolean,
    sun_allergy: boolean,
    fast: boolean | null,
    fear: boolean | undefined
  })
);

I don't see the reason, why this test should fail, but as you can see, it does, because 'fast' and 'fear' are expected to be of 'null' and 'undefined' types respectively.

These errors are popping out all over the place and cause pain to work with propEq.

[Harris-Miller]

@Nemo108 I checked and rechecked what we need to do and why. Both your MRs at a high-level 3 things

• Better typing for val when we don't yet know the type for obj
• Full currying
• Handling when obj is an array

That is a lot to check, verify, and agree on in a single MR. I would like to suggest we split these into 3 MRs, and focus on one part at a time.

I made the first MR that just focuses on the first case: #74

Please let me know what you think of my analysis to the problem and my proposed solution

[Harris-Miller]

Given the happenings in both #99 and #73. I went back to this MR to look at the changes made that we left off on. Given the now known limitations about trying to do the other improvements (particularly with arguments, possibly returning never there, and cross function type support), what is in this MR currently is the best option I think to move forward with in the short-term.

The code changes do not solve all the issues called out, but it's better than nothing. I'm going to give it a thorough re-test and see if we can try and get this merged after #99

[Harris-Miller]

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

[Harris-Miller]

• 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

[Harris-Miller]

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;

[Harris-Miller]

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;
	};

[Harris-Miller]

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;