should deepMixIn and deepFillIn merge arrays?

[millermedeiros]

right now arrays are also merged which can be a good thing or a bad thing depending on the scenario.

https://github.com/mout/mout/blob/master/tests/spec/object/spec-deepFillIn.js#L75-L83 and https://github.com/mout/mout/blob/master/tests/spec/object/spec-deepMixIn.js#L79-L109

maybe all the deep methods should only accept 2 objects and we add a 3rd argument to control the behavior. Or maybe we even create separate methods.

[satazor]

@millermedeiros If we rethink this from 0, I tend to disagree with having deepMixIn and deepFillIn.

I think mootools got this right.

Object.merge - Merges any number of objects recursively without referencing them or their sub-objects.
This should be our mixIn - which do not mess with arrays and stuff like that.

And our fillIn should be the same, except that it fills in into the first object recursively.

If the user really wants the current behaviour of mixIn (which does not work recursively), why don't they simply do it themselves? Is actually very simple to do a forOwn or for in + hasOwn.

[satazor]

Actually mootools has Object.append that does what the current mixIn does.
Object.append - Copies all the properties from the second object passed in to the first object passed in.

Having this in mind, it makes sense to leave mixIn, fillIn, deepMixIn, deepFillIn but remove the merge of arrays.

[conradz]

I would agree with not merging arrays. Merging arrays seems a little too "magical". If someone wants to merge arrays they should probably define how to do it themselves. I would be -1 on a third parameter. Possibly a config object would work, but how many times do we need to clone arrays at all?

[satazor]

Yes it's strange that an object method like deepMixIn and deepFillIn to be handling arrays deep inside the object. If the user really wants it to be done he should do it manually.
If we support this via a third parameter I think we should also remove the ability to pass multiple arguments from the normal mixIn and fillIn for consistency of the API. But in general I'm against the 3rd argument, but I understand if it gets added.

[millermedeiros]

Imagine a scenario like this:

var base = {
    btn_submit : 'Submit',
    btn_cancel : 'Cancel',
    sliders : [
        {
            id : 'income',
            label : 'Income',
            min : 0,
            max : 500000
        },
        {
            id : 'inflation',
            label : 'Inflation Rate',
            min : -0.5,
            max : 10
        }
    ]
};

var pt = {
    btn_submit : 'Enviar',
    btn_cancel : 'Cancelar',
    sliders : [
        {
            label : 'Salário'
        },
        {
            label : 'Inflação'
        }
    ]
};

var config = locale === 'pt'? deepMixIn({}, base, pt) : base;

I know it's error-prone (would be better to use an object instead of an array) but in some cases user might want to merge all items of the array as well. Sometimes we have no control over JSON responses.

But I guess you guys are right, replacing the array would be the most common behavior. @satazor also had some interesting thoughts about Date, RegExp and custom types (that they should not be merged but replaced instead).

[satazor]

@millermedeiros I expect sliders to be the one from the second object but you actually want it merged. I think that those cases require manual merging.
It feels strange that a function inside of the object package is actually merging arrays.

What about creating a lang/merge ?

[millermedeiros]

let's just kill the array merge and replace anything that isn't a plain object. that way we also solve issues with #1 - Date, RegExp, Array, MyCustomConstructor would all be replaced without merging the properties.

[conradz]

I'm starting to think that we should maybe have some configuration for these methods. We could possibly even merge (no pun intended) deepMixIn and deepFillIn, and have the fillin behavior specified in the configuration. Not sure how to specify the configuration, since the easiest would be a config object which would also be an object ...

If no configuration, I think killing all the "magic" would be good.

[millermedeiros]

lets kill the "magic" for now, we can always add more methods if needed. it's too common to use mixIn with more than 2 objects (specially since it mutates the first object).

[satazor]

Can this be closed or there is some missing tests?