[Task]: Replace cloneDeep with klona

rishabhrathod01 commented 2 years ago

Is there an existing issue for this?

[X] I have searched the existing issues

Description

As klona is faster when compared to lodash's cloneDeep. We would like to migrate from cloneDeep to klona in all the places where it is possible in client code.

Steps

[ ] Search for cloneDeep in client code
[ ] Replace cloneDeep with klona
[ ] Add tests to verify you changes works as expected before and after the change.

alzaar commented 2 years ago

Hi! I would like to give this a shot. I see in the code, there are multiple instances of the cloneDeep function call so may take me some time. Please let me know, if I can take this on.

rishabhrathod01 commented 2 years ago

Hey @alzaar, thanks for showing interest 🎉 , This is all yours. Assigning this to you now.

Please don't forget to read the Contribution Guidelines.

Also, if you need any help feel free to reach out to us on our discord.

Araxeus commented 2 years ago

I've recently tested all clone implementations I could find, here are the results for 1_000_000 runs each: (klona or klona_light are the most reliable & fast)

name	time.ms	time.ops	errors.0	errors.1	errors.2
klona_json	369.076	2709471	Date object wasn't cloned	Regex object wasn't cloned	Point.toString wasn't cloned
copyFastestJsonCopy	535.425	1867675	Point.toString wasn't cloned	assert.deepEqual error
rfdc	668.025	1496949	Point.toString wasn't cloned	assert.deepEqual error
klona_light	810.074	1234455
klona	957.160	1044757
deep_copy	1131.280	883955	Point.toString wasn't cloned	assert.deepEqual error
tily	1132.424	883061	Point.toString wasn't cloned
nanoclone	1387.806	720562	Point.toString wasn't cloned
nanoCopy	1688.971	592077
cloneDeep	1812.293	551787	Point object wasn't cloned
just_clone	1834.962	544970	Point.toString wasn't cloned
copy_anything	2535.151	394454	Date object wasn't cloned	Regex object wasn't cloned	Point object wasn't cloned
fastCopy	2547.182	392591
ramda	2947.646	339254
klona_full	3545.780	282025
lodashCloneDeep	3915.724	255381
clone	4127.941	242252
structuredClonePolyfill	5865.863	170478	Point.toString wasn't cloned
structuredClone	5906.439	169307	Point.toString wasn't cloned

Test fixture is:

class Point {
    constructor(x, y, alive) {
        this.x = x;
        this.y = y;
        this.alive = alive;
    }

    toString() {
        return 'POINT' + this.x;
    }
}

export const fixture = {
    i: 1,
    o: {
        a: [1, 2, 3, 4, 5],
        o: {
            s: 'test123',
            r: /aworw/g,
            d: new Date(),
            o: {
                i: 1898983759325,
                b: {},
                n: null
            },
            p: new Point(1, 2, true),
            //c: Point
        },
        n: null,
    },
    s: 'sdf',
    u: undefined
};

if you want the benchmark code, I could upload it too :P

alzaar commented 2 years ago

Thanks @Rishabh-Rathod for assigning it to me. Yea, already looking into discord Is there a strict requirement to use one over the other ? klona or klona_light ?

rishabhrathod01 commented 2 years ago

if you want the benchmark code, I could upload it too :P

Hey 👋 @Araxeus, Yes, even when we analyzed klona and its benchmark looked great to us. Here, is a link to PR where we added klona. https://github.com/appsmithorg/appsmith/pull/12568

It would be great if you could make your benchmark code open source and add a link here or in the above PR. :)

@alzaar As we are already using klona, there won't be any need to install a library. You could go ahead with replacing cloneDeep with klona. :)

Araxeus commented 2 years ago

Here you go 🙂 https://github.com/Araxeus/deep_clone_benchmarks

rishabhrathod01 commented 2 years ago

Hey @alzaar,

Are you still working on this? We might unassign the issue if it is not being worked on. We will wait for a few days and then go ahead with unassignment.

alzaar commented 2 years ago

Hey @Rishabh-Rathod ,

No, I am not. I struggling with the debugging process on my side. I posted in the discord channel but I am still on the learning curve for typescript. You can unassign me for this at the moment. If nobody is working on it couple of months from now, I will post here again.

Thanks, Ali

berabhishek commented 2 years ago

I would like to work on this

rishabhrathod01 commented 2 years ago

Greetings, thanks for showing interest 🎉 , This is all yours. Assigning this to you now. @berabhishek

berabhishek commented 2 years ago

Update: Working on this, will update the PR this week

rishabhrathod01 commented 2 years ago

@berabhishek as there could be many cloneDeep in code base. This could be done in parts removing few in one PR and so on.

berabhishek commented 2 years ago

Update: Replaced all cloneDeep with klona, added test cases for a couple of them If it is fine, we can merge this and add test cases in subsequent prs.

Vishrut19 commented 1 year ago

Hey! Can I work on it?

yash9871 commented 11 months ago

hey, can i give it a shot ?

sak-90 commented 11 months ago

Please assign this to me if this hasn't been closed yet. Thank you.

berabhishek commented 11 months ago

Yes sure, feel free to take it up

On Sun, 22 Oct, 2023, 1:43 am sak-90, @.***> wrote:

Please assign this to me if this hasn't been closed yet. Thank you.

— Reply to this email directly, view it on GitHub https://github.com/appsmithorg/appsmith/issues/12936#issuecomment-1773911928, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACK73A7C3XRBUXRJJVUFID3YAQUFZAVCNFSM5TN4AEXKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCNZXGM4TCMJZGI4A . You are receiving this because you were mentioned.Message ID: @.***>

JaiZemoso commented 3 months ago

This issue is not closed yet, even the raised PR has not merged, CAn u assign in it to me @berabhishek

berabhishek commented 3 months ago

I don;'t think i can. assign anyone, @rishabhrathod01 I have unassigned myself

JaiZemoso commented 3 months ago

Can u kindly assign this issue to me @rishabhrathod01

In my local I have successfully done my changes on replacing cloneDeep with Klona, it will take very less time for me to close this issue. Kindly assign me this issue

rishabhrathod01 commented 3 months ago

@JaiZemoso Thanks for showing your interest in contributing. The steps mentioned in the description are outdated. Give us some time to update them for a hassle-free contribution. Meanwhile please look for other Good First issues.

JaiZemoso commented 3 months ago

@JaiZemoso Thanks for showing your interest in contributing. The steps mentioned in the description are outdated. Give us some time to update them for a hassle-free contribution. Meanwhile please look for other Good First issues.

@rishabhrathod01 @marks0351 I have created a fresh pull request for this issue, can you knidly check?

PR Details: https://github.com/appsmithorg/appsmith/pull/34477

appsmithorg / appsmith