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

feat: try native structuredClone in cloneDeep #5855

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lewxdev
Copy link

@lewxdev lewxdev commented Apr 29, 2024

resolves #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.

@kapilkumar9395
Copy link

Instead of having skipNativeCheck as a parameter, it would be better to have an option object something like -
cloneDeep(value, { skipNativeCheck : false })

It's more scalable, and descriptive

@Trott
Copy link

Trott commented Apr 29, 2024

The yarn.loock update seems unrelated?

@lewxdev
Copy link
Author

lewxdev commented Apr 29, 2024

Instead of having skipNativeCheck as a parameter, it would be better to have an option object something like - cloneDeep(value, { skipNativeCheck : false })

It's more scalable, and descriptive

That works, I just figured using an object for a single option would be unnecessary (and I didn't see too many other uses of options for the other utilities).

But the scalability argument makes sense and I'm open to making this change

@lewxdev
Copy link
Author

lewxdev commented Apr 29, 2024

The yarn.loock update seems unrelated?

Yeah, the yarn.lock got updated when I did bun i. Maybe I can restore the old version and try again with --frozen-lockfile?

EDIT: Turns out this didn't work. not sure what the issue is, I can remove the lockfile changes on request

@lewxdev lewxdev force-pushed the #5833-cloneDeep branch 2 times, most recently from 79dcf39 to ff2c89f Compare April 29, 2024 16:35
src/cloneDeep.ts Outdated Show resolved Hide resolved
src/cloneDeep.ts Outdated Show resolved Hide resolved
Copy link

@TheJaredWilcurt TheJaredWilcurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the yarn lock, but the rest looks good to me.

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.
@TheJaredWilcurt
Copy link

@jdalton I think this is good. Thoughts on the yarn lock?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Attempt structuredClone in cloneDeep before doing manual clone
4 participants