Fixed #6 (reuses object in deep assignment)

[Kreozot]

No description provided.

[Kreozot]

@sindresorhus, I draw your attention that "support symbols as targets" check was failed before this change (when I ran tests just after cloning and now on master branch).

[schnittstabil]

@Kreozot Thank you for your suggestion, but I think it is incompatible with the project description of deep-assign:

Recursive Object.assign()

That is because Object.assign() does not distinguish plain and non-plain objects, which is the reason I've created merge-options.

[Kreozot]

Sorry, but I don't get it. How do you think deep-assign should behave with internal objects? Because without this PR it acts like simple object-assign.

@sindresorhus, what do you say?

[schnittstabil]

@Kreozot It does not. Think of the following snippet already mentioned in #6:

function User(name) {
  this.name = name;
}

var target = {
    user: new User('Alice')
};

var source = {
    user: new User('Bob')
};

// target.user exists, so deep-assign steps into target.user and source.user:
deepAssign(target, source);
//=> target.user.name === source.user.name === 'Bob'

This shows that the target object tree is recursively updated with properties of source, i.e.:

target.user.name = source.user.name;
// but: target.user !== source.user

With non-recursive Object.assign:

Object.assign(target, source);
//=> target.user === source.user

[Kreozot]

Yes. But with this bug it acts exactly like this. When you assign fixture.foo.bar it actually assign only fixture.foo (that's why changing bar affects original object)

[schnittstabil]

@Kreozot With the last example I just wanted to show that deepAssign does not act exactly like Object.assign, and that it is recursive.

[Kreozot]

That's why I said "...with this bug..."
This PR makes deep-assign works as you showed in your example.

[schnittstabil]

@Kreozot I thought my (deleted) example was flawed by itself (the output doesn't match the code and I couldn't have recognized any difference).

Took me some time to find some real difference:

var alice = {};
Object.defineProperty(alice, 'name', {value: 'Alice'});

var result = deepAssign({}, {user: alice});
//Current => result.user.name === 'Alice'
//PR      => result.user.name === undefined

Personally I would prefer the current behavior, because it is consistent with non-plain objects:

function User() {}
var alice = new User();
Object.defineProperty(alice, 'name', {value: 'Alice'});

var result = deepAssign({}, {user: alice});
//Current => result.user.name === 'Alice'
//PR      => result.user.name === 'Alice'

In addition, also the ECMAScript® 2015 Language Specification does not distinguish plain and non-plain objects.

[Kreozot]

Finally got it. Thanks for the explanation. Will think.

[Kreozot]

Sooo... The problem is not in the PR. The problem is in for (var key in from) { statement. If you change your property definition to this
Object.defineProperty(alice, 'name', {value: 'Alice', enumerable: true});
it will be completely allright.

[schnittstabil]

@Kreozot exactly, the enumerable makes the difference.

The problem is not in the PR

How you look at it: Your PR introduces the inconsistenties between plain and non-plain objects I've mentioned.

[Kreozot]

But the whole point of object assign is that it clone object instead of copying a pointer. Without the PR this is useless at all. I can't be sure that object, from that I assign, is safe.

[Kreozot]

Maybe we should create another issue about this problem (and a test for it of course)?

[schnittstabil]

But the whole point of object assign is that it clone object

I don't think so.

1. from 19.1.2.1 Object.assign ( target, ...sources ):

   1. c. 3. Let status be Set(to, nextKey, propValue, true).

   There is no cloning mentioned, only setting, especially ToObject (i.e. Object()) does not clone:

   var obj = {};
   var objRef = obj;
   obj = Object(obj);
   //=> objRef === obj

2. all properties are assigned to target, i.e. neither target nor the sources are cloned:

   var target = {};
   var targetRef = target;
   Object.assign(target, {foo: 1});
   target = Object.assign(target, {bar: 2});
   //=> targetRef === target

3. in most cases cloning is not possible (in a reasonable way), e.g.

   function target(n) {
     return true;
   }
   function source(n) {
     return n % 2;
   }
   Object.assign(target, source);
   //=> target !== source
   
   target(0) //=> true
   target(1) //=> true

I can't be sure that object, from that I assign, is safe.

Sometimes it is intended not to clone and share references.

[Kreozot]

If we talk about object-assign, I agree. But isn't deep-assign do the same but for all nested properties?

[alexykot]

I support @Kreozot in here. I've installed deep-assign for this very reason to recursively copy an object, but found it not working as expected. I will have to use his fork instead.

[stevemao]

We need to defined what "Recursive Object.assign()" really means here. And I think @Kreozot is more intuitive.

[sindresorhus]

This module is now deprecated: b332062