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

O.Compact broken in 6.13 #132

Closed
mesqueeb opened this issue Jul 20, 2020 · 7 comments
Closed

O.Compact broken in 6.13 #132

mesqueeb opened this issue Jul 20, 2020 · 7 comments
Labels
bug Something isn't working

Comments

@mesqueeb
Copy link
Contributor

🐞 Bug Report

import { O } from 'ts-toolbelt'

export enum ACTIVE_LESSON_STUDENT_STATUS {
	NEEDS_HELP = "NEEDS_HELP",
}

export interface StudentMeta {
	statuses: ACTIVE_LESSON_STUDENT_STATUS[]
}

type MergedWithCompact = O.Compact<StudentMeta, [Partial<StudentMeta>], 'deep'>

// v6.12 ✅
// type MergedWithCompact = {
// 	statuses: ACTIVE_LESSON_STUDENT_STATUS[];
// }

// v6.13 ❌
// type MergedWithCompact = {
// 	statuses: {
// 			[x: number]: ACTIVE_LESSON_STUDENT_STATUS;
// 	};
// }

The 0.13 is breaking a lot of things in my libraries with ts-toolbelt as depedency.

Right now my solution is to hard-code the version to v0.12 in all my libraries & tools that use ts-toolbelt.

@millsp
Copy link
Owner

millsp commented Jul 20, 2020

Looking into this

@millsp millsp added the bug Something isn't working label Jul 20, 2020
@millsp
Copy link
Owner

millsp commented Jul 20, 2020

This is happening because the new Merge is able to merge lists too, the old one would just skip it. This was a huge issue for type safety. And the problem here is that I chose to set the merging default to ramda style, that means that lists are not preserved by default...

So if you enable lodash-merging style, you can do pretty amazing things!

type t0 = Merge<{
    a: number[]
    b: {a: 1}[]
    c: {a: [0]}
}, {
    a: string[]
    b: {b: 2}[]
    c: {a: [0, 1, 2]}
}, 'deep', 0>

// {
//     a: (string | number)[];
//     b: {
//         a: 1;
//         b: 2;
//     }[];
//     c: {
//         a: [0, 1, 2];
//     };
// }

Actually, I plan to make the lodash-merging style the default soon.

@mesqueeb
Copy link
Contributor Author

mesqueeb commented Jul 20, 2020

@millsp
I'm all for positive change and increasing possibilities!

How do we enable "Lodash-merging style"? I've never used Lodash before. 😅

@millsp
Copy link
Owner

millsp commented Jul 21, 2020

You just have to pass the 0 option like in the example. 1 for ramda.

@millsp
Copy link
Owner

millsp commented Jul 21, 2020

Waiting for this to merge DefinitelyTyped/DefinitelyTyped#46213

@millsp
Copy link
Owner

millsp commented Jul 21, 2020

The only thing to know when using lodash merging style is that when one of your fields is undefined, lodash will fill that gap when merging, that's not the case for ramda. Ramda instead checks if the key exists or not. So if you have a field that you explicitly set to undefined, ramda wont complete it when merging, lodash will.

@millsp
Copy link
Owner

millsp commented Jul 27, 2020

Fixed!

@millsp millsp closed this as completed Jul 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants