dojo / core

:rocket: Dojo 2 - language helpers and utilities.
http://dojo.io
Other
213 stars 62 forks source link

Confusion about lang.deepMixin and other deep... (urgent) #290

Closed sebilasse closed 7 years ago

sebilasse commented 7 years ago

Preclaimer: What I need is the old deepCopy which is currently

"in an odd location"

in dojo1, see http://dojo-toolkit.33424.n3.nabble.com/deep-lang-mixin-td4007082.html but where is it in dojo2 ???

lang/deepMixin and lang/deepAssign behave totally different from deepCopy (same behaviour like plain /mixin for me), check Dylans example https://jsfiddle.net/dylan/oja45dv8/

I would expect lang/deepMixin to return the same than the old deepCopy or did I overlook sth. ?

agubler commented 7 years ago

@sebilasse Do you mind providing code examples with expected results against actual results for the @dojo/core/lang#deepAssign or @dojo/core/lang#deepMixin modules? And perhaps give some details how they behave differently to dojo1's deepCopy?

sebilasse commented 7 years ago

@agubler Sorry. That was why I included Dylans Fiddle – The expected result is exactly the current fiddle result (in the console) :

Let us deep mix

{
  apple: 0,
  banana: { weight: 52, price: 100 },
  cherry: 97
};

with

{
  banana: { price: 200 },
  durian: 100
};

---> Expected Result (as per dojo1)

{
  apple: 0,
  banana: { weight: 52, price: 200 },
  cherry: 97,
  durian: 100
}

which is all the properties deep mixed together...

In dojo2 the wrong result is

{ 
  apple: 0, 
  banana: { price: 200 }, 
  cherry: 97, 
  durian: 100 
}

Now I don't know the weight of the banana anymore. I would not buy that banana ;))

sebilasse commented 7 years ago

@agubler @dylans I found the bug see https://github.com/dojo/core/blob/master/src/lang.ts : line 67

In _mixin if it shouldDeepCopyObject the target is always an empty object …

The easiest fix I could think of is to change line 67 to target: (!!target && target[key])||{}

sebilasse commented 7 years ago

Could we also discuss an example with arrays ? See forked https://jsfiddle.net/7yL7sgj1/ - confusing ...

agubler commented 7 years ago

@sebilasse fix up at https://github.com/dojo/core/issues/292