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

feat(zipObj): stricter types, add test #88

Merged
merged 4 commits into from
Apr 2, 2024

Conversation

GerkinDev
Copy link
Contributor

Narrow values types per key when possible, otherwise fallback on wider mapped type.

Tests included

@GerkinDev GerkinDev marked this pull request as draft January 27, 2024 00:47
@GerkinDev GerkinDev marked this pull request as ready for review January 27, 2024 01:00
@Harris-Miller
Copy link
Collaborator

Harris-Miller commented Jan 28, 2024

While I understand what you're changes do, I'm struggling to find the use case for it.

What you're additions are handling for tuples, but not only tuples, strict subtypes within (eg 'foo' over string, 1 over number).

Was hoping you could provide some

@Harris-Miller
Copy link
Collaborator

To dive into specifically when I mean here... all your unit tests utilize tuples in that each index of the array is strongly typed to be separate:

zipObj(['bool', 'number', 'null', 'undefined'], [true, 1, null, undefined])

This use case is extremely rare. Even you did receive back from an API this way, as a list of key/value tuples [['name', string], ['age', number], ['gender', 'M' | 'F' | 'X']], you would almost always know the key/value type pairing in advanced and would have defined a type for them

type Types = {
  'bool': boolean;
  'number': number;
  'null': null;
  'undefined': undefined;
};

type Person = {
  'name': string;
  'age: number;
  'gender': 'M' | 'F' | 'X';
};

My concern over this additional typing is the complexity of it. And while yes we could do it this way, doesn't mean we actually should. And in this case, I think the complexity of extracting an exact type over exactly defined tuples is not worth it. Type casting does the trick just the same since you can define the exact type of the object ahead of time as well

const person = zipObj(['bool', 'number', 'null', 'undefined'], [true, 1, null, undefined]) as Person; // gives you what you'd need 99% of the time

I'm making some assumptions here of course, as I don't know your actually use course, and I'm more than happen to entertain it and discuss. But right now my gut is telling me that adding the additional complexity is not worth support a <1% edge case of usage

@GerkinDev
Copy link
Contributor Author

GerkinDev commented Jan 28, 2024

While I agree with your concern, my use case was based on pipe. I used zipObj as a part of a pipe with multiple processings, where typing each functions/params would be very verbose and unpractical. And in that situation, the current implementation would return something like this (assuming #89 is merged):

// Might be slightly wrong, but you got the idea
const splitBools = pipe(
  partition((v):v is boolean => typeof v === 'boolean'),
  zipObj(['bools', 'others'])
)

const splitted = splitBools(['a', 1, true])
type Expected = {bools: boolean[], other: Array<number | string>}
type Actual = {
    bools: boolean[] | (string | number)[];
    others: boolean[] | (string | number)[];
}

Also, narrowing 2nd parameter as tuple when possible won't actually narrow to literals most of the times, since the 2nd param will be wider at usage.

@GerkinDev
Copy link
Contributor Author

Hello, do you need anything for this PR ? Or should I close it ?

@Harris-Miller
Copy link
Collaborator

Please keep it open. I've been meaning to come back to it to review. I wanted to get your other MR as well as mine for isNil merged and released first.

@Harris-Miller
Copy link
Collaborator

I also have a much better idea why you wanted this changes and how the complexity helps from those other MRs. So I'm not still apposed to these changes as I initially called out as a concern. Now it's just a point to review the changes themselves. I'll try to block out some time before the end of this week to do that

@GerkinDev
Copy link
Contributor Author

There's no rush, I'm a FOSS maintainer too, I know what it is. I just wanted to be sure this PR has a chance to be useful


export type _ZipObj<K extends readonly PropertyKey[], V extends {[key in keyof K]: unknown} | readonly unknown[]> =
Copy link
Collaborator

Choose a reason for hiding this comment

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

This utility type cannot live in this file. The build script combines all the files into one and copies the ramda block comments over. It forces us to have one export function per file

Don't add it to src/types/util/tools.d.ts though. I want to break that file up into a bunch of smaller ones (who has the time, right?)

Instead, do it like I have src/types/util/deepModify.d.ts by creating a new file src/types/util/zipObj.d.ts.

test/zipObj.test.ts Show resolved Hide resolved
@Harris-Miller
Copy link
Collaborator

I made a key assumption about the complexity of the typings in that it would return something like this:

const r = zipObj(['foo', 'bar'], [1, 2]);
//    ^? _.L.ZipObj<['foo', bar'], [1, 2]>

Those types coming out of a library that can take plan array/object types in and not return them back out is not a goot user experience and is something I'm trying to avoid where I can.

To my present surprise, it actually returns plan types!

const r = zipObj(['foo', 'bar'], [1, 2]);
//    ^? { foo: 1, bar: 2 }

I tested your changes against the existing tests defined in DefinitelyTyped and all passed

@GerkinDev
Copy link
Contributor Author

Just as a note, if you need a hand for some type definitions, and since I love TS types, don't hesitate to ping me somehow, maybe by creating an issue.

@GerkinDev
Copy link
Contributor Author

GerkinDev commented Mar 26, 2024

Hey there, hope you're doing well ! Would it be possible to have this merged soon ?

@Harris-Miller Harris-Miller merged commit 3976be8 into ramda:develop Apr 2, 2024
3 checks passed
@GerkinDev GerkinDev deleted the zipObj branch April 2, 2024 06:46
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