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

Revert "Update: propEq to allow wider-typing for value in comparison" #99

Merged
merged 1 commit into from
Mar 2, 2024

Conversation

Harris-Miller
Copy link
Collaborator

Reverts #74

@Nemo108 In my MR out on , this comment pointed out a test that I had commented out. I had the intention of coming back to, but forgot (human error).

That test is this:

    const coll = [{ type: "BUY" }, { type: "SELL" }, { type: "BUY" }];
    const isBuy = R.propEq("BUY", "type");
    R.filter(isBuy, coll); // => [{ type: 'BUY' }, { type: 'BUY' }]

You get an error on isBuy being passed to R.filter:

Expand to see error ``` No overload matches this call. Overload 1 of 5, '(pred: (val: { type: string; }) => val is { type: string; }, list: readonly { type: string; }[]): { type: string; }[]', gave the following error. Argument of type '>>(obj: Required extends Record<"type", any> ? string extends WidenLiterals ? U : never : never) => boolean' is not assignable to parameter of type '(val: { type: string; }) => val is { type: string; }'. Signature '(obj: { type: string; }): boolean' must be a type predicate. Overload 2 of 5, '(pred: (val: unknown) => val is unknown, dict: Record): Record', gave the following error. Argument of type '>>(obj: Required extends Record<"type", any> ? string extends WidenLiterals ? U : never : never) => boolean' is not assignable to parameter of type '(val: unknown) => val is unknown'. Types of parameters 'obj' and 'val' are incompatible. Type 'unknown' is not assignable to type 'Partial>'. Overload 3 of 5, '(pred: (value: unknown) => boolean, collection: { type: string; }[]): { type: string; }[]', gave the following error. Argument of type '>>(obj: Required extends Record<"type", any> ? string extends WidenLiterals ? U : never : never) => boolean' is not assignable to parameter of type '(value: unknown) => boolean'. Types of parameters 'obj' and 'value' are incompatible. Type 'unknown' is not assignable to type 'Partial>'.ts(2769) ```
It's very long and hard to understand. tl;dr is that the function signature returned by `R.propEq("BUY", "type")` is not compatible as a first argument for `R.filter()`. Or in other words, typescript does it see it as a `(pred: (val) => boolean)`

I'm pretty sure it's because of how we have the turnary and the : never. The fact that that function's first argument could be the bottom type never, it will never be compatible with R.filter() or any function that is looking for a a function predicate such at that.

This means that #74 breaks existing behavior in a net-negative way. While it did solve a lot of the defined issues called out, I can't keep this as is.

This sucks, a lot. But I must revert it. I can't move forward if it's going to break consumer's existing code negatively like this.

@Harris-Miller Harris-Miller merged commit 1595fed into develop Mar 2, 2024
3 checks passed
@Harris-Miller Harris-Miller deleted the revert-74-propEq branch March 2, 2024 04:05
@kurtinatlanta
Copy link

kurtinatlanta commented Mar 4, 2024

This is the second time in the last couple weeks that a change to the propEq typings has caused numerous downstream issues for me. Can we please stop messing with this in a patch release as it causes breakage downstream and is not by any means backward compatible.

Thanks.

@Harris-Miller
Copy link
Collaborator Author

@kurtinatlanta First, let me apologize for our typing updates giving you so much trouble. It's not our intention. I personally messed up with propEq. tl;dr is that how I normally test the develop branch against DefinitelyTyped before releasing gave me a false positive because of how they switched from npm to pnpm. So I released types-ramda with failing tests.

I don't have that flow captured anywhere, but I'll add it to the readme. I'm also going to add an issue to get some some Gitlab Actions created that adds testing directly against DefinitelyTyped

To address patch... Unfortunetly we're stuck with patch releases both here and for @types/ramda. This project and that one is not synced with core ramda. And we cannot diverge from major.minor. We can only diverge patch

eg. ramda is 0.29.1 currently, if we want to do type fix, we have to release 0.29.x always. We can't do 0.30.x until the core package bumps to 0.30.x as well. We have to keep those synced. This is true for all "community" typings, and is no isolated to us.

@kurtinatlanta
Copy link

Thanks. I get that stuff happens from time-to-time. I'm sure you'll get it all worked out.

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