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

[WIP] fix(propEq): improve propEq typings #73

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

Conversation

Nemo108
Copy link
Contributor

@Nemo108 Nemo108 commented Oct 15, 2023

  • 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
Copy link
Collaborator

@Nemo108 I finally got around to re-checking and merging #71. Rebase this one for me and I'll recheck it

@Nemo108
Copy link
Contributor Author

Nemo108 commented Jan 21, 2024

@Harris-Miller done


type Obj = {
union: 'foo' | 'bar';
str: string;
num: number;
int: number;
numLike: number | `${number}`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

${number} is a neat trick! Allows strings that only contain number characters, very cool

};
export function propEq<K extends keyof U, const U>(__: Placeholder, name: K, obj: U): (val: WidenLiterals<U[K]>) => boolean;
export function propEq<K extends keyof U, const U>(val: WidenLiterals<U[K]>, __: Placeholder, obj: U): (name: K) => boolean;
export function propEq<K extends keyof U, const U>(val: WidenLiterals<U[K]>, name: K, 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.

We don't want to WidenLiterals<U[K]> here. When we know the type of U, we want to constrain val to U[K]

type Obj = {
  prop: 'foo' | 'bar'
};

// we want to disallow strings other than `'foo'` and `'bar'` here for `val`
propEq('biz', 'prop', obj);

WidenLiterals<U[K]> is needed for the curried versions before of how typescript infers types

const doesEqualFoo = propEq('foo', 'prop');

// without `WidenLiterals<U[K]>`, the type would be `Record<'prop', 'foo'>`, but obj is `{ prop: 'foo' | 'bar' }`, which is too wide for just `'foo'`.
// with `WidenLiterals<U[K]>`, the type would be `Record<'prop', string>`, so `obj` is allowed
doesEqualFoo(obj)

Typescript doesn't support currying like this that well in general, but we do the best we can with what we have

};
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): K extends number ? {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of the ternary, we do 2 overloads

export function propEq<K extends number>(__: Placeholder, name: K): {
  // ...
}
export function propEq<K extends Exclude<PropertyKey, number>>(__: Placeholder, name: K): {
  // ...
}

? (array: Array<WidenLiterals<V>>) => boolean
: (obj: Record<K, WidenLiterals<V>>) => boolean;
<const U>(__: Placeholder, obj: U): (name: keyof U) => boolean;
<K extends keyof U, const U>(name: K, 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.

I don't think const U in needed here. U should be find.

You need to constrain U and K here though. If typeof U[K] as well. Turns out you don't need WidenLiterals here at all, in fact, it would actually break if U[K] was string | number but you passed a string to val: V.

See here: https://tsplay.dev/m3KbAm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

V extends U[K] ? U : never creates too strict constraint: it fires an error on any string I pass there

https://tsplay.dev/wOXozN

But I think I came up with a better solution

* 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
Copy link
Collaborator

@Nemo108 I'm having to revert the other propEq we collaborated on together: #74 (see: #99)

This MR is actually of of develop before that merge, so after the revert happens there should be no more merge conflict.

The additions made to #74 solved a different problem about propEq. I pretty sure the changes in this MR are find to merge without those changes. I'm also pretty sure that the specific change of #74 that broke it (argument possibly being never) not present in these changes, so we shouldn't have that problem again

Comment on lines +1 to +2
import { Placeholder } from 'ramda';
import { WidenLiterals } from '../util/tools';
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
import { Placeholder } from 'ramda';
import { WidenLiterals } from '../util/tools';
import { Placeholder, WidenLiterals } from '../util/tools';


export function propEq(__: Placeholder): never;
export function propEq<const V>(val: V): {
<K extends number>(name: K): <U extends any[]>(array: V extends U[K] ? V[] : never) => boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah crap, I missed this when I left that other comment about this MR not being a problem

array: V extends U[K] ? V[] : never

This is the thing we can't have. It breaks combining with other functions, egR.filter(R.propEq('type', 'something')). The predicate type signatures don't match because of the : never

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

Successfully merging this pull request may close these issues.

2 participants