Ts-toolbelt: `Object.Merge` with `deep` doesn't work on arrays/lists

Created on 8 Jul 2020  路  42Comments  路  Source: millsp/ts-toolbelt

馃悶 Bug Report

Is your feature request related to a problem?

I'm trying to create a type from a deep merge of various types.
When these types include arrays the merge stops working.
I've tried Object.Assign, Object.AssignUp, Object.Merge, Object.MergeUp, and Object.Compact, none of them worked for my issue.

Describe the solution you'd like

Here's a simplified example:

Given these types:

type Person = {
  type: "Person";
  name: string;
};

type Pet = {
  type: "Pet";
  kind: "dog" | "cat";
};

type HasPet = {
  pet: Pet;
};

I want to create a new type:

type Merged = {
  people: {
    type: "Person";
    name: string;
    pet: {
      type: "Pet";
      kind: "dog" | "cat";
    };
  }[];
}

This is what I tried:

import { O } from "ts-toolbelt";

type Merged = O.Compact<
  {},
  [
    {
      people: Person[];
    },
    {
      people: HasPet[];
    }
  ],
  "deep"
>;

I also tried replacing Compact with the aforementioned alternative types, to no avail.

This example, and another one both exist in a CodeSandbox: https://codesandbox.io/s/pensive-franklin-3xn39, if you want to play around with it.

bug needs investigation

Most helpful comment

You could have a CreativeGroupWithInstanceDataList that is a tuple instead of an union, this way you can have a list like you wanted. You'd need to reorganize the way your type is built from the deepest field up to its top parent.

I will also add an option so that CompactUp and AssignUp can be used with lodash merging style.

I see the new Compute/MergeUp are causing problems, so I will fix this very soon as well. But it's almost there. Writing such types is actually very complex, so please excuse me for the bugs!

Will get back to you soon.

All 42 comments

Hi there, this was supposed to be performed by https://pirix-gh.github.io/ts-toolbelt/modules/_object_p_merge_.html, but it is not working like intended.

Thanks for reporting this. I'll look into it asap.

@bengry this will work for you:

type t0 = MergeUp<
    [{b:聽2},聽{d: 4}],
    [{c:聽3},聽{e:聽5}],
'deep', 0>

type t1 = MergeUp<
    Person[],
    HasPet[],
'deep', 0>

The option 0 means that we'll be using lodash-style merging. Under the hood, AssignUp and CompactUp use MergeUp but don't provide a way to set the merge style to either ramda or lodash due to technical limitations and performance. Ramda is always the preferred merging style.

Though I still have to investigate why the tools in O.P are failing to handle such a scenario. Let me know if there is anything else you need to know. I'll keep the issue open until issues on O.P are solved.

Cheers

@pirix-gh thanks! I'll look into it. From an initial look though this doesn't solve my use case in the real-world app. I'll see about recreating a more true-to-original example in CodeSandbox and update later.

As a side note, what's the difference between Assign vs. Merge vs. Compact, and also vs. Assign vs AssignUp (or Merge vs MergeUp, or Compact vs CompactUp, I assume they're all the same logical difference between one another)? I didn't see this being explained in the docs, or maybe I just missed it.

I will be glad to help you with a more complex example. Here's how your first example would work:

import {A, O} from 'ts-toolbelt'

type Person = {
    type: 'Person';
    name: string;
};

type Pet = {
    type: 'Pet';
    kind: 'dog' | 'cat';
};

type HasPet = {
    pet: Pet;
};

type test0 = A.Compute<O.MergeUp<Person[], HasPet[], 'deep', 0>>

The Up versions are the couterparts that are able to deal with unions and optional fields. In essence MergeUp will behave like the spread operator while Merge will only merge fields that do not exist already. This might disappear in the upcoming v7 to make the Up versions the default.

At this moment, all utilities ending by Up use MergeUp under the hood. I also updated the docs for AssignUp, CompactUp,Assign and Compact to make it clearer. They all use Merge or MergeUp to merge batches (lists) of types.

Thanks. Here's a more complex example that more resembles my real world use case: https://codesandbox.io/s/pensive-franklin-3xn39?file=/src/complex.ts

It's a lot bigger though, so I hope it's not too overwhelming, I tried to remove a lot of our code from there to make it smaller (the actual version is a lot more complex).

Essentially, I want to create a new type that has everything in it:

interface Merged {
  id: string;
  channel: MediaChannelSource;
  campaigns_meta: readonly CampaignMeta[];
  instanceDataStatus: "pending" | "done";
  instances: ReadonlyArray<{
    id: string;
    channel: MediaChannelSource;
    campaign_ids: readonly number[];
    campaigns_meta: readonly CampaignMeta[];
    data: "pending" | CreativeTypesData<MediaChannelSource>;
  }>;
}

It's a bit more nuanced than the above type, since the type of instances[number]['channel'] depends on the one from Merged['channel'], and so does the Merged['instanceDataStatus'] affecting Merged['instances'][number]['data'] (the latter being not pending when the former is "done"), but I hope this makes sense.

At this stage, your type looks like

type CreativesGroupWithCampaignsAndData = {
    campaigns_meta: readonly CampaignMeta[];
    instances: {
        [x: number]: {
            id: string;
            channel: "facebook";
            campaign_ids: number[];
            data: {
                fb_post_text: string;
            };
        };
        campaigns_meta: CampaignMeta[];
    };
    id: string;
    time: Date;
    channel: "facebook";
    localField: string;
    instanceDataStatus: 'done';
} | {
    campaigns_meta: readonly CampaignMeta[];
    instances: {
        [x: number]: {
            id: string;
            channel: "youtube";
            campaign_ids: number[];
            data: {
                yt_video_title: string;
            };
        };
        campaigns_meta: CampaignMeta[];
    };
    id: string;
    time: Date;
    channel: "youtube";
    localField: string;
    instanceDataStatus: 'done';
}

Wrap your types with A.Compute (update to the latest version for best results).

It looks like it's working, it's just that it is not strictly equals to what you are testing (readonly CampaignMeta[]). You basically did:

type t0 = CreativesGroupWithCampaignsAndData['instances'][number]

// which is equal to
type t1 = {
    id: string;
    channel: "facebook";
    campaign_ids: number[];
    data: {
        fb_post_text: string;
    };
} | {
    id: string;
    channel: "youtube";
    campaign_ids: number[];
    data: {
        yt_video_title: string;
    };
}

For some reason, you are looking for a field campaigns_meta that is not in [instances][number]. It is situated in instances:

type t2 = CreativesGroupWithCampaignsAndData['instances']['campaigns_meta']

I'm pushing another update for A.Compute to help you vizualize your types more easily.

For some reason, you are looking for a field campaigns_meta that is not in [instances][number]. It is situated in instances:

type t2 = CreativesGroupWithCampaignsAndData['instances']['campaigns_meta']

instances is an array, it wouldn't make a lot of sense for campaigns_meta to be on it (i.e. a sibling property to .length). It should be a property on each instance (object in the array).

I'm actually getting most properties as never ([email protected]).

type CreativesGroupWithCampaignsAndData = Any.Compute<
    CampaignsMeta &
        Object.MergeUp<
            { instances: CampaignsMeta },
            Union.Select<
                CreativeGroupWithInstanceData,
                { instanceDataStatus: "done" }
            >,
            "deep",
            0
        >
>;

Here's a repo you can clone, CodeSandbox has some weird things with TS I think: https://github.com/bengry/ts-deepmerge

Ok, then I think you've made a mistake:

export type CreativesGroupWithCampaignsAndData = A.Compute<CampaignsMeta &
    Object.MergeUp<
      { instances: CampaignsMeta },
      Union.Select<CreativeGroupWithInstanceData, { instanceDataStatus: 'done' }>,
      'deep',
      0
    >>;

You are merging array-things into a non-array, you probably forgot:

export type CreativesGroupWithCampaignsAndData = A.Compute<CampaignsMeta &
    Object.MergeUp<
      { instances: CampaignsMeta[] },
      Union.Select<CreativeGroupWithInstanceData, { instanceDataStatus: 'done' }>,
      'deep',
      0
    >>;

Now this works:

type t3 = CreativesGroupWithCampaignsAndData['instances'][number]['campaigns_meta']

And you get never values because you are using the & operator. That's a known behavior of this operator, it must absolutely not be used for merging, use the ts-toolbelt for this :)

Ok, then I think you've made a mistake:
...
You are merging array-things into a non-array, you probably forgot:

Yep, my bad there. Thanks!

And you get never values because you are using the & operator. That's a known behavior of this operator, it must absolutely not be used for merging, use the ts-toolbelt for this :)

I replaced the & with O.MergeUp and still get never:

export type CreativesGroupWithCampaignsAndData = Any.Compute<
    Object.MergeUp<
        CampaignsMeta,
        Object.MergeUp<
            { instances: CampaignsMeta[] },
            Union.Select<
                CreativeGroupWithInstanceData,
                { instanceDataStatus: "done" }
            >,
            "deep",
            0
        >
    >
>;

image

Also pushed the latest changes to the aforementioned GitHub repo, if you want to take a look there.

Update to the latest version in the meantime, Compute will give you more fine-grain details. I'm looking into this.

I don't get this, on my side:

type CreativesGroupWithCampaignsAndData = {
    campaigns_meta: readonly {
        campaign_id: number;
        campaign_name: string;
    }[];
    instances: {
        campaigns_meta: {
            campaign_id: number;
            campaign_name: string;
        }[];
        id: string;
        channel: "facebook";
        campaign_ids: {
            [x: number]: number;
        };
        data: {
            fb_post_text: string;
        };
    }[] | {
        campaigns_meta: {
            campaign_id: number;
            campaign_name: string;
        }[];
        id: string;
        channel: "youtube";
        campaign_ids: {
            [x: number]: number;
        };
        data: {
            yt_video_title: string;
        };
    }[];
    id: string;
    time: Date;
    channel: "youtube" | "facebook";
    localField: string;
    instanceDataStatus: 'done';
}

Remove your node_modules, reinstall with ts>3.9 and the latest ts-toolbelt. And let me know :)

Hey @bengry I don't see your repo anymore so I'm marking this as resolved.

Hey @bengry I don't see your repo anymore so I'm marking this as resolved.

I created it as private by mistake (default from the gh cli) 馃う. Just fixed, so you should be able to see it now.

Remove your node_modules, reinstall with ts>3.9 and the latest ts-toolbelt. And let me know :)
Just did, using: [email protected] and [email protected], same thing as before...

Please re-open.

P.S.

I'll keep the issue open until issues on O.P are solved.
Is the issue with O.Merge fixed?

Yep, that is solved, I got mistaken, it was working as intended. What you would like, though, might be a O.P.MergeUp that does not exist yet.

Any ideas regarding what's not working in the repo I linked to?

Busy looking into it

It seems to be a problem on my side, that only happens when 0 (lodash-like merging) is enabled. Will ping you when I have a fix later (soon).

Misbehaviour repro:

type t2 = Object.MergeUp<{
    a: 1
}, {a: 2}, 'deep', 0>

I found why this is happening. It comes from the fact that you don't have strictNullChekcs flag enabled.

I'm currently trying to see if I can find a workaround for this.

I found why this is happening. It comes from the fact that you don't have strictNullChekcs flag enabled.

We do use strict everything in the actual app. I forgot to initialize a tsconfig in the repro repo. Will do that in a bit and see if it gets a better result.

It is not possible without this flag, you will have to enable it. I updated the docs to warn about this.

There are a few problems still that I am tracking down

R1, R2, R3 are now passing

Looks much better, thanks!
I'm still not getting the behavior I want though, not sure if this is a bug in ts-toolbelt or in my definitions though:

declare const merged: CreativesGroupWithCampaignsAndData;
if (merged.channel === 'facebook') {
    const i = merged.instances[0].channel;
    // I expect i to be of type `'facebook'` (a group can only contain instances of the same channel).
}

IIRC this worked before?

Just to recap, I'm using the same definition as before:

export type CreativesGroupWithCampaignsAndData = Any.Compute<
    Object.MergeUp<
        CampaignsMeta,
        Object.MergeUp<
            { instances: CampaignsMeta[] },
            Union.Select<
                CreativeGroupWithInstanceData,
                { instanceDataStatus: "done" }
            >,
            "deep",
            0
        >
    >
>;

I'm also seeing weird behavior with Dates in my code (I didn't add it in the contrived example, I'm now seeing it's a special case though):

This is especially an issue when you want to pass this object to something that expects a Date (e.g. moment(myDate)).
I'm also seeing a similar issue with arrays:

If you prefer, I'd be more than happy to do a call over Zoom (or similar), or a VS Code Live Share, if you've got the time and energy for that. Then I can show you the actual repo and code, which are similar to what I uploaded, but more complicated. Maybe this will shorten the feedback loop.

I'm still not getting the behavior I want though, not sure if this is a bug in ts-toolbelt or in my definitions though:

This is normal, CreativeGroupWithInstanceData is a union, so even if it's merged it's not going to have indexes. I've been looking into how to do this for an hour now and I don't find any easy way, there are too many constraints. I suggest that you redesign the way you structure your type by making CreativeGroupWithInstanceData a tuple instead of a union... Or at least have a tuple that can represent those two entries, its name does not actually matter. Unions cannot be transformed to arrays reliably, they won't preserve the order. This is making what you want to achieve impossible, another way has to be found.

I'm going to work on fixing those errors with Date and other built-in objects, this is pretty common that ts destructs built-in objects.

Support for built-on objects with MergeUp will land tomorrow.

You can now use the MergeUp utility with the deep option enabled, it will preserve all built-in objects.

I'm closing this issue as all the problems related to this lib have been resolved. I really recommend that you restructure your types so that you can end up with a tuple instead of a union. Feel free to ask more questions, and thanks for the bug reports.

@millsp How would a tuple work in this case?

Also, on a related note - is the a way to achieve the type we talked about using Object.Compact/other similar type - that accepts a list of types to merge? Otherwise I have to call Object.MergeUp twice, which is fine, but seems like it could be optimized.

Thanks for all the help!

You could have a CreativeGroupWithInstanceDataList that is a tuple instead of an union, this way you can have a list like you wanted. You'd need to reorganize the way your type is built from the deepest field up to its top parent.

I will also add an option so that CompactUp and AssignUp can be used with lodash merging style.

I see the new Compute/MergeUp are causing problems, so I will fix this very soon as well. But it's almost there. Writing such types is actually very complex, so please excuse me for the bugs!

Will get back to you soon.

@millsp I understand that it's very complex, no worries. What types of problems? I didn't see anything specific from my usage, but I'm surely not seeing the big picture - just my project.

Also, I'm still not sure what do you mean by a tuple. Could you give a code example with the concrete type I'd have, similarly to what I wrote here: https://github.com/millsp/ts-toolbelt/issues/124#issuecomment-656207514, how would that look with a tuple?

@bengry, MergeUp did not deal with unions properly like it should. I'm working on making MergeUp the default since I'm not satisfied with Merge anymore. I'm also working on adding the merging options at the same time.

Will come back to you soon.

You can now use MergeAll or PatchAll as you need. This will be out in a few minutes. I'll help you with your code tomorrow.

You can now use MergeAll or PatchAll as you need. This will be out in a few minutes. I'll help you with your code tomorrow.

Thanks! I'll give it a try tomorrow. What's PatchAll? I didn't see it in the docs (or even something similar to it).

Oh that's because the docs are still being built on the CI. PatchAll is exactly like MergeAll but it does not handle undefined and optionals, it just completes the missing fields of a type. And btw, MergeUp does not exist anymore, it's called Merge (same for all Up utilities, they were confusing and a misconception).

And Merge behaves exactly like the spread operator, but for types, it handles merging, unions, undefined, and optional fields too. This lib is the only one providing such a type so far.

I just fixed a last bug that was causing the types to end up as never (mentioned above). Now it's about your code @bengry. You should start heading into a direction where you work with:

export type CreativeGroupWithInstanceDataList =
    [
        CreativeGroupWithInstanceDataGeneric<'facebook'>,
        CreativeGroupWithInstanceDataGeneric<'youtube'>
    ];

Then, I guess you would want to perform a Select on that List like you did before:

export type CreativeGroupWithInstanceDataListStatusDone<L = CreativeGroupWithInstanceDataList> = {
    [K in keyof L]: Union.Select<L[K], { instanceDataStatus: 'done' }>
};

Note: This <L = CreativeGroupWithInstanceDataList> has to be this way for ts not to iterate over array methods. Nevermind.

And if you want to get your original CreativeGroupWithInstanceData, you can just do:

export type CreativeGroupWithInstanceData =
    List.UnionOf<CreativeGroupWithInstanceDataList>;

This is as far as I can help you, I tried to dive in your code again and I am not sure why you are doing those data structures transformations. Try to restructure your types so that you can achieve the result you need. Good luck!

I would like to help you more but I am very busy. You can always fund an issue on issue hunt then I will allocate you some hours. But I'm sure you'll do great :)

Was this page helpful?
0 / 5 - 0 ratings

Related issues

JeremyJonas picture JeremyJonas  路  6Comments

thesunny picture thesunny  路  4Comments

qwelias picture qwelias  路  3Comments

regevbr picture regevbr  路  4Comments

mesqueeb picture mesqueeb  路  5Comments