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

a bug fix in assginDeep.js #5

Closed
wants to merge 1 commit into from
Closed

Conversation

yanCode
Copy link

@yanCode yanCode commented Jul 12, 2017

as described in this ticket, #4 , now I have got some time and provide my fix to it.

@bcherny
Copy link
Owner

bcherny commented Jul 12, 2017

Thanks for the contribution @yanCode! Does the code have the same problem for other class/function instances? If so, would it be better to use a simplified version of what lodash does?

@yanCode
Copy link
Author

yanCode commented Jul 12, 2017

@bcherny , thanks for your reminder of testing other types, I did some tests, it turns out that the array type in a property still fails, like assignDeep({a: 1}, {b: [1,2,3]}).
which is a bit tricky, when it recursively calls the assignDeep on an array, if this array passes the test isObject, the assigned array turns to be {'0':1, '1':2, '2': 3}. on the other hand, if this array doesn't pass the isObject test, then the array is assigned using '=', well this is shallow copy.

the loadash implementation, still treats an array as an object. _.isObject([1,2]) === true

my conclusion is that it needs to add some specific logic to deep copy the array.

@bcherny
Copy link
Owner

bcherny commented Jul 12, 2017

You want _.isPlainObject, not _.isObject.

In any case, this seems to be the wrong path to go down. I've updated the blog post:

assignDeep - Like Object.assign, but merges objects deeply. For the sake of simplicity, you can assume that objects can contain only numbers and other objects (and not arrays, functions, etc.).

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

Successfully merging this pull request may close these issues.

None yet

2 participants