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

Attempt structuredClone in cloneDeep before doing manual clone #5833

Open
TheJaredWilcurt opened this issue Mar 18, 2024 · 6 comments · May be fixed by #5855
Open

Attempt structuredClone in cloneDeep before doing manual clone #5833

TheJaredWilcurt opened this issue Mar 18, 2024 · 6 comments · May be fixed by #5855

Comments

@TheJaredWilcurt
Copy link

I would assume that the structuredClone API being native would allow JS engine makers to optimize the performance. Though it isn't as powerful as Lodash's clone deep, that power may not be needed in all use cases.

My suggestion is basically this:

function cloneDeep (value) {
  try {
    if (structuredClone) {
      return structuredClone(value);
    }
  } catch {
    return doWhatLodashHasAlwaysDoneForCloneDeep(value);
  }
}

Just an idea, not sure if this would be a net positive or negative (more people dealing with the time it takes to do the failed attempt before falling back, versus more people getting the fast native solution and skipping the more complex JS solution).

Alternatively every user could just make their own helper function like this:

import { cloneDeep } from 'lodash.cloneDeep';

export const deepClone = function (value) {
  try {
    if (structuredClone) {
      return structuredClone(value);
    }
  } catch {
    return cloneDeep(value);
  }
};

But the whole point of Lodash it to not need to make/copy a bunch of common helpers for stuff like this.

@jdalton
Copy link
Member

jdalton commented Apr 11, 2024

YES YES YES! @TheJaredWilcurt up for creating a PR?

@kapilkumar9395
Copy link

How we are going to check for complex object? Structured Clone Algorithm doesn't support functions and other things.

@Trott
Copy link

Trott commented Apr 22, 2024

How we are going to check for complex object? Structured Clone Algorithm doesn't support functions and other things.

It seems like this question is answered in the code suggestion by OP. Trying to clone a function will throw, and fall back to the current implementation.

@kapilkumar9395
Copy link

That is correct. I overlooked try catch.

@TheJaredWilcurt
Copy link
Author

YES YES YES! @TheJaredWilcurt up for creating a PR?
- @jdalton

Unfortunately I don't think I will be able to any time soon. Despite what the VC-Backed Bun claims, it in fact does not actually run on Windows. I don't know if I'll have time to clone lodash down on one of my Linux VMs and also try to get bun working on them.


Implementation Ideas

I would request, for whoever does implement this feature, that you allow passing in an option into cloneDeep to skip the native check. Since sometimes I have objects I know will fail the native try/catch, so I could save some time by skipping that attempt. I imagine others would appreciate this option too.

@lewxdev
Copy link

lewxdev commented Apr 28, 2024

YES YES YES! @TheJaredWilcurt up for creating a PR?

Unfortunately I don't think I will be able to any time soon. Despite what the VC-Backed Bun claims, it in fact does not actually run on Windows. I don't know if I'll have time to clone lodash down on one of my Linux VMs and also try to get bun working on them.

Implementation Ideas

I would request, for whoever does implement this feature, that you allow passing in an option into cloneDeep to skip the native check. Since sometimes I have objects I know will fail the native try/catch, so I could save some time by skipping that attempt. I imagine others would appreciate this option too.

I can try implementing this PR 👍🏽

lewxdev added a commit to lewxdev/lodash that referenced this issue Apr 29, 2024
lewxdev added a commit to lewxdev/lodash that referenced this issue Apr 29, 2024
lewxdev added a commit to lewxdev/lodash that referenced this issue Apr 29, 2024
resolves lodash#5833

I tried to add tests but they don't seem to be running at the moment,
this can be adjusted as necessary

Any feedback on the proposed approach and any adjustments or additions
is greatly appreciated. Additionally, insights on enhancing the
implementation are welcome as well.
@lewxdev lewxdev linked a pull request Apr 29, 2024 that will close this issue
lewxdev added a commit to lewxdev/lodash that referenced this issue Apr 29, 2024
resolves lodash#5833

I tried to add tests but they don't seem to be running at the moment,
this can be adjusted as necessary

Any feedback on the proposed approach and any adjustments or additions
is greatly appreciated. Additionally, insights on enhancing the
implementation are welcome as well.
lewxdev added a commit to lewxdev/lodash that referenced this issue Apr 29, 2024
resolves lodash#5833

I tried to add tests but they don't seem to be running at the moment,
this can be adjusted as necessary

Any feedback on the proposed approach and any adjustments or additions
is greatly appreciated. Additionally, insights on enhancing the
implementation are welcome as well.
lewxdev added a commit to lewxdev/lodash that referenced this issue Apr 29, 2024
resolves lodash#5833

I tried to add tests but they don't seem to be running at the moment,
this can be adjusted as necessary

Any feedback on the proposed approach and any adjustments or additions
is greatly appreciated. Additionally, insights on enhancing the
implementation are welcome as well.
lewxdev added a commit to lewxdev/lodash that referenced this issue Apr 30, 2024
resolves lodash#5833

I tried to add tests but they don't seem to be running at the moment,
this can be adjusted as necessary

Any feedback on the proposed approach and any adjustments or additions
is greatly appreciated. Additionally, insights on enhancing the
implementation are welcome as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants