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
[Utils] mergedeep deeply clones source key if it's an object #35364
Conversation
|
5529b78
to
e8b77ff
Compare
packages/mui-utils/src/deepmerge.ts
Outdated
@@ -1,3 +1,5 @@ | |||
import cloneDeep from 'lodash/cloneDeep'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lodash is a no go.
import cloneDeep from 'lodash/cloneDeep'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliviertassinari I'm agree, it has been added to illustrate an solution and check that all tests will be passed. Should we write own implementation of clonedeep or do we have other ways?
e8b77ff
to
9776461
Compare
packages/mui-utils/src/deepmerge.ts
Outdated
function clonedeep<T>(source: T): T | Record<keyof any, unknown> { | ||
if (!isPlainObject(source)) { | ||
return source; | ||
} | ||
|
||
const output: Record<keyof any, unknown> = {}; | ||
|
||
Object.keys(source).forEach((key) => { | ||
output[key] = clonedeep(source[key]); | ||
}); | ||
|
||
return output; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliviertassinari Seems that this implementation fits well, all tests passed and code coverage is happy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to be working well. I've got a small remark.
packages/mui-utils/src/deepmerge.ts
Outdated
@@ -6,6 +6,20 @@ export interface DeepmergeOptions { | |||
clone?: boolean; | |||
} | |||
|
|||
function clonedeep<T>(source: T): T | Record<keyof any, unknown> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you call this function deepClone
? I know that deepmerge
doesn't follow conventions (should be deepMerge
), but let's ensure new functions are named consistently with most of the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaldudak sure
6b041aa
to
a9a5287
Compare
a9a5287
to
34dd04d
Compare
@michaldudak @oliviertassinari branch is updated and ready for review. Thank you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All good! Thanks for your contribution!
I'm not sure about
lodash
but we definetly have to use deep copy for objects otherwise value assign by reference.Test case has been added.
Fix #35363