Skip to content
This repository has been archived by the owner on Jan 23, 2021. It is now read-only.

fix: deep assign should ignore inseparable object #7

Closed
wants to merge 1 commit into from
Closed

fix: deep assign should ignore inseparable object #7

wants to merge 1 commit into from

Conversation

haoxins
Copy link

@haoxins haoxins commented Oct 31, 2015

No description provided.

@haoxins
Copy link
Author

haoxins commented Oct 31, 2015

also fix failed test: support Object.create(null) targets

@schnittstabil
Copy link
Contributor

Thank you @coderhaoxin, but that's not how Object.assign works, e.g:

// this PR:
> deepAssign({regexp: /reg/}, {regexp: 'string'});
{regexp: 'string'}

// but:
> Object.assign(/reg/, 'string')
{ /reg/ '0': 's', '1': 't', '2': 'r', '3': 'i', '4': 'n', '5': 'g' }

@@ -137,7 +165,7 @@ test('support `Object.create(null)` objects', t => {
test('support `Object.create(null)` targets', t => {
var obj = Object.create(null);
obj.foo = true;
t.same(fn(obj, {bar: false}), {foo: true, bar: false});
t.same(fn({}, obj, {bar: false}), {foo: true, bar: false});
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing is wrong with this test. The first deep-assign argument is the target and this test asserts, that the following does not throw an Error:

deep-assign(Object.create(null), )

Copy link
Contributor

Choose a reason for hiding this comment

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

@coderhaoxin Thank you for mentioning that. You were right, this test had to be repaired (e252fa2).

@haoxins haoxins closed this Oct 31, 2015
@haoxins
Copy link
Author

haoxins commented Oct 31, 2015

@schnittstabil Oh, I miss something. Thx

@haoxins haoxins deleted the ignore-inseparable-object branch October 31, 2015 11:59
@haoxins
Copy link
Author

haoxins commented Oct 31, 2015

@schnittstabil Object.assign() convert all the arguments to Object, but should deep-assign the same?

I mean, is it reasonable that

deepAssign({regexp: /reg/}, {regexp: 'string'});

should equal to

Object.assign(/reg/, 'string')

for the property regexp?

@schnittstabil
Copy link
Contributor

The project description of deep-assign says:

Recursive Object.assign()

@coderhaoxin Therefore I would say yes, in my opinion it is the canonical way.

But I must admit that the behavior is not very obvious, which is shown by #3, #4, #6. I am open to new interpretations of recursive and assign, but deep-assign should behave like Object.assign in the base case…

@schnittstabil
Copy link
Contributor

@coderhaoxin merge-options might be of interest to you.

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

Successfully merging this pull request may close these issues.

None yet

2 participants