add depth parameter

[matthewkastor]

A depth parameter to limit how many nodes deep to go.

[nullivex]

• :+1:

This would be excellent for me to have available.

[matthewkastor]

This would definitely require the addition of a parameter to the function. I'd implement it as an options object to facilitate future enhancements. This would require bumping the major version number because it would break anything using the current version of the module. I'm not opposed to introducing this here but I want to think about it a little more. Since object-merge can be used to clone objects, adding a depth parameter would be creating a broken, object version of array.slice. I might as well go the whole way, offering a start depth, end depth, and a caveat that negative "indexes" aren't supported... at least at first.

I'm going to have a busy morning tomorrow. :D

[nullivex]

I would appreciate it lots and would make my package a lot safer.

If you need some help or want me to work on some PR's for it let me know. Thanks!

[matthewkastor]

I don't ever get upset over people wanting to help. :D If you've got something send it over. I release everything under the gpl 3, so as long as that's not an issue for you we can get this figured out.

[nullivex]

I didnt have a chance to fork the repo this evening. Had a mess of other things going on however I was pondering the usage and I was thinking this.

//new usage
var myObject = objectMerge([myObject,obj1,obj2,obj3],{depth: 5, circularCheck: false})
//backwards compat (probably wont work because the above look the same but might be able to make it fall back?)
var myObject = objectMerge(myObject,obj1,obj2)

I suppose the only problem with that is that the new arguments look that same as the old arguments both could be Array / Object.

Even if it involves breaking the backwards compatibility I like the idea of objectMerge([Array of Objects],{Options}) as the prototype.

I setup this branch until we get things ironed out: https://github.com/snailjs/object-manage/tree/object-merge. In the morning I will setup a few tests that pass bad objects.

[matthewkastor]

You're right, there isn't really a good way to differentiate between the options object and other objects that might look the same. It looks like this new parameter would be required. If the options object comes before the rest of the arguments then the options could be bound and, using the bound function would be the same as using object-merge right now.

For instance, if normal usage were to be

objectMerge({Options}, obj...);

Then it could be made backward compatible with

var objectMergeB = objectMerge.bind(null, {Options});
objectMergeB(obj...);

Is there any reason you want the options object as the last parameter? I could do it either way but it makes more sense to me that the options are specified first to allow for creating partials.

[nullivex]

Well if it were first could it be omitted to have defaults?

[matthewkastor]

The only way I'd be able to make it optional would be to stop allowing an unlimited number of arguments. There are a lot of useful things that can be done with a function that accepts unlimited args of the same type. The only way I'd be able to make the options optional would be to require the objects to be merged be packed into an array or something. With the way it's written now, it can be called or applied but if I changed the definition to take just an object and an array it would be less versatile. Do you know what I'm saying? I don't know if I'm expressing myself very well.

[nullivex]

Yes I do.

So what about this.

objectMerge.set({depth: 5, circularCheck: false})
objectMerge(obj1,obj2)

I believe to do that you just add a set prototype to the functions and store the options in a private variable to the closure should work and backwards compat?

[matthewkastor]

Hmm... When you say "private to the closure" you're talking about the implied closure for the module right? I could make this work with something like the code below. Try it out in your console and let me know if it's structured like what you're describing.

'use strict';

function getModule() {var exports = {};
    var opts = {
        depth : 0
    };
    function foo () {
        console.log('merging objects using depth of: ' + opts.depth);
    }
    function _set (obj) {
        obj = obj || {};
        obj.depth = obj.depth || opts.depth;
        obj.depth = (typeof obj.depth === 'number') ? obj.depth : 0;
        opts.depth = obj.depth;
        return opts;
    }
    function _get () {
        return opts;
    }
    function _showOpts () {
        console.log(
            JSON.stringify(opts, null, '    ')
        );
    }
    Object.defineProperties(foo, {
        options : {
            set : _set,
            get : _get,
            configurable : false,
            enumerable : true
        },
        set : {
            value : _set,
            configurable : false,
            enumerable : true,
            writable : false
        },
        showOpts : {
            value : _showOpts,
            configurable : false,
            enumerable : true,
            writable : false
        }
    });
    exports = foo;
return exports;}

// sort of like require
var faux = getModule();

// these properties are not writable so they can't be changed.
// With strict mode turned off they still can't be reassigned
// but no error is thrown.
try {
    faux.showOpts = function () {return null};
} catch (e) {
    console.log(e.message);
}
try {
    faux.set = 'xd';
} catch (e) {
    console.log(e.message);
}

faux.showOpts();
faux();
faux.options.depth = 10;
faux.showOpts();
faux();
faux.set({depth:20});
faux.showOpts();
faux();
faux.options = {banana:'yes'};
faux.showOpts();
faux();
faux.set({depth:'tomato'});
faux.showOpts();
faux();

[nullivex]

Here is what I was thinking (roughly based off your example)

'use strict';
function getModule(){
  var exports = {}

  var depth = -1

  exports = function(merge){
    console.log('depth: ' + depth)
    console.log('merging:',merge)
  }

  exports.set = function(options){
    depth = options.depth || -1
  }

  exports.showOpts = function(){
    console.log('depth: ' + depth)
  }

  return exports
}

var objectMerge = getModule()

objectMerge({foo: 'fooo'})
objectMerge.set({depth: 10})
objectMerge({foo: 'foo2'})
objectMerge.showOpts()

[matthewkastor]

I was thinking about it today and this configuration would lead to a race condition. The depth setting would be "global" to the module so asynchronous calls to object-merge with different settings for the depth option would override each other. The return results would be weird if the value of depth were changed in the middle of a merge, which could very well happen here. In order to do this right, object-merge would have to be made into a class so it could contain it's own settings and there's not a good way to wrap it so it would look like a regular function call.

So, at this point the best options for adding this in appear to be either redefining object-merge with a required options parameter, or converting it into a class.

var objectMerge = require('object-merge');
var options = {
    depth : 10
};
objectMerge(options, obj, obj, ...);

or

var ObjectMerge = require('object-merge');
var merger = new ObjectMerge({optional settings});
merger.options = {
    depth : 10
};
merger.merge(obj, obj, ...);

For the sake of making it easier to update my code that's already using this, I'm leaning more toward not converting this function into a class. All I'd have to do is run a regex on my code to insert null as the first argument everywhere I've called objectMerge and everything will just work. If I turn it into a class I'll have to do a bunch of updates by hand. Still, I'm open to both. I wouldn't want to make something useless just because it's easy for me. Which way do you like better, function or class?

[nullivex]

Well my whole point with the property usage was that the defaults should be backwards compatible with a -1 depth which should be unlimited.

I also dislike being required to pass an options object first as the defaults will work fine 99% of the time.

[matthewkastor]

Passing in an options object is unavoidable unless I change this into a class. The first idea for tracking options only works if those options won't change throughout program execution but I can easily think of cases where users would want to limit the depth of merging on one set of objects and not on another. A global setting just won't work in this case and we're stuck with either passing the options into a function or instantiating a whole class to track the specific details about how this, now method, should be executed on different sets of objects. Both options allow for specifying defaults so there's no issue with that.

The issue with keeping this a function is that there isn't any reliable way to differentiate between an object to be merged and an options object. Keep in mind that object-merge handles the case where a scalar value is merged with an object, so even specifying that "if the first arg is a number, take it to be the depth option" won't work. I take that back. There is a way to differentiate between objects using instanceof but, the solution is pretty convoluted. It involves creating a constructor for an options object and passing an instance of the options object into object-merge in order to set the options... optionally.

function dump (obj) {
    return JSON.stringify(obj, function(k,v){
            if(typeof v === 'function') {
                return v.toString();
            } else {
                return v;
            }
        }, '   ');
}

function ObjectMergeOptions () {
}

function merger () {
    var out = {}, options = {depth:-1}, mergeSet = [];
    if(arguments[0] instanceof ObjectMergeOptions) {
        options = arguments[0];
        mergeSet = Array.prototype.slice.call(arguments, 1);
    } else {
        mergeSet = Array.prototype.slice.call(arguments, 0);
    }
    if(mergeSet.length === 0) {
        console.log('Options : \r\n' + dump(options) + '\r\n\r\nMerging : \r\n' + dump(mergeSet));
        //return out;
    } else {
        console.log('Options : \r\n' + dump(options) + '\r\n\r\nMerging : \r\n' + dump(mergeSet));
        //return out;
    }
}

// example objects
var foo = {mergeFoo:true,fn:function bar(){return null;}};
var bar = {mergeBar:true};

// creates an options object
var opts = new ObjectMergeOptions();
// with Object.defineProperties, setters and getters can be implemented to
// do sanity checks on assignments. No need for setter and getter methods
// it will just look like regular assignments to properties on the object.
opts.depth = 5;

// does not consider argument 0 to be options
merger({depth:5}, foo, bar);

// does consider argument 0 to be options
merger(opts, foo, bar);

You know, now that I've written it down this seems like a decent solution. It would be completely backward compatible, satisfy your desire to have optional options, not require instantiation of object-merge for each different set of options and, it's simple to implement. I think I like it. What do you think, is requiring that the options object be a specific kind of object an elegant solution? I think this would satisfy both of our requirements and leave object-merge as simple to use as possible. If you like it let me know tonight and I'll probably be able to get it set up and tested.

[nullivex]

+1 :+1:

I like it, best solution yet.

[matthewkastor]

I've started some work on this. The options object can be created and is recognized. The only option working at the moment is for disabling circular reference checks. The depth option can be set, there's just nothing in the code yet that uses it. My family is going to be over here soon for Christmas so I don't think I'll be working on it anymore tonight. Take a look at the tests, there is one that's failing right now ('object-merge allows specifying depth of traversal') but once the depth option is used and things start going right then it should pass.

[matthewkastor]

ok, they were taking longer than I thought so I finished it up. Try it out when you get a chance.

[nullivex]

Haha, ya I have been sitting here coding a bit too. Ill give it a shot now.

[nullivex]

Okay.

Awesome!!! I like this a lot. Also by adding the limit of the depth I got a significant performance increase.

Old

    ◦ should warn on objects more than 50 levels deep:
    √ should warn on objects more than 50 levels deep (144ms)

  24 passing (162ms)

New

    ◦ should warn on objects more than 50 levels deep:
    √ should warn on objects more than 50 levels deep

  24 passing (20ms)

Here is my usage which was nice and straightforward :)

/**
 * Merge implementation using object-merge
 * @param obj1
 * @param obj2
 * @returns {*|exports}
 */
ObjectManage.prototype.objectMerge = function(obj1,obj2){
  var objectMerge = require('object-merge')
  var opts = objectMerge.createOptions({
    depth: this.maxDepth,
    throwOnCircularRef: true
  })
  return objectMerge(opts,obj1,obj2)
}

[nullivex]

Is the new version on NPM? I just sucked down the master but if this is released I will go ahead and update and release my package too.

[matthewkastor]

I don't know what you're benchmarking exactly but it looks like having the ability to stop traversing the objects saves you a considerable amount of time. This is great news, it means we've done something useful for the nerds who come after us. :D

The latest version is on NPM, with updated docs and a fancy new section in the readme. :D For your merge method above, you might want to check the values supplied for obj1 and obj2. I don't know what you want the behavior to be but having one or the other undefined might not give you what you want.

var opts = objectMerge.createOptions({
    depth: false,
    throwOnCircularRef: true
});
var foo = {a:'a'};

console.log(
    undefined === objectMerge(foo, undefined)
    &&
    JSON.stringify(foo) === JSON.stringify(objectMerge(undefined, foo))
);

I'm going to close this issue because it seems we've figured out exactly how to solve it and our tests are calling our solution good. Thanks for bringing this up and taking the time to brainstorm with me.

[nullivex]

The library confirms they are objects before passing down to the objectMerge function.

I believe the benchmark gain is from the deeper object not recursing the full 100 levels deep.

Yeah, everything seems good and I appreciate you taking the time to help me out. I was trying to avoid writing yet another merge package just to tackle a few niggles.

[matthewkastor]

:+1: