2

I am learning to build a Javascript constructor and met the following problem:

var x = new Constructor({
        B: '1000',
        child: document.getElementById('id'), // assume there is a div#id
    });

function Constructor(options) {
    var settings;

    this.defaults = {
        A: 'value A',
        B: 2,
        parent: document.body,
        child: document.getElementById('anotherId'), // assume there is a div#anotherId
    }

    settings = extend({}, this.defaults, options);

    console.log(this.defaults.A)      // outputs 'value A'
    console.log(this.defaults.B)      // outputs 2
    console.log(this.defaults.parent) // outputs <body>...</body>
    console.log(this.defaults.child)  // outputs <div id="id">...</div>

    console.log(settings.A)           // outputs 'value A'
    console.log(settings.B)           // outputs '1000'
    console.log(settings.parent)      // outputs empty object
    console.log(settings.child)       // outputs empty object
}

function extend(out) {
    out = out || {};
    for (var i = 1; i < arguments.length; i++) {
        var obj = arguments[i];

        if (!obj) continue;

        for (var key in obj) {
            if (obj.hasOwnProperty(key)) {
                out[key] = (typeof obj[key] === 'object') ? extend(out[key], obj[key])
                                                          : obj[key];
            }
        }
    }
    return out;
}

The problem is settings.parent doesn't have the same value as this.defaults.parent, but the other properties are just fine.

What could be the problem here?

yqlim
  • 6,898
  • 3
  • 19
  • 43
  • 1
    I don't understand. What's the point of recursing if it's an object? – Andrew Li Mar 02 '17 at 03:39
  • 1
    @AndrewLi this can be important in some cases when copying settings because when you pass in an option whose value is an object, you don't necessarily want that object to be mutated. So deep-copying the options object prevents that. – Matt Browne Mar 02 '17 at 05:04

1 Answers1

2

The problem is the extend() function - it apparently wasn't written to support copying DOM nodes like document.body. I recommend using Object.assign() instead - it's a native function available in ES6 and there are shims available for older browsers (such as this one on MDN).

Usage:

settings = Object.assign({}, this.defaults, options);

Note that this only makes a shallow copy, so might not work in some situations. If you are copying nested objects and you want to be sure that you're not modifying the original object in this.defaults or options, then you might need to do some deep copying / cloning.

I'm not sure where you got that extend() function, but it does do some basic deep copying. The problem is that DOM nodes can't be cloned in the same way as regular objects. There is a way to clone them - you call the clone() method - but the purpose of that is to create an actual copy of the DOM node that you can then add somewhere else in the document, which isn't what you want to do here.

Are you using any DOM library such as jQuery? If so you could do this and avoid the whole problem:

this.defaults = {
    A: 'value A',
    B: 2,
    parent: $('body'),
    child: $('#anotherId'), // assume there is a div#anotherId
}

This would work because jQuery objects are regular JS object that wrap one or more DOM nodes.

UPDATE: Actually for this to work you'd still need to modify the extend() function or use a different cloning function, because there's another bug in extend(). See "UPDATE" below.

Alternatively you could alter your extend() method so it only copies references to DOM nodes but deep-copies all other kinds of objects. Here's how you could do that:

function extend(out){
    out = out || {};
    for (var i = 1; i < arguments.length; i++){
        var obj = arguments[i];
        if (!obj) continue;
        for (var key in obj){
            if (obj.hasOwnProperty(key)) {
                if (typeof obj[key] === 'object') {
                    //don't deep-copy DOM nodes
                    if (obj.nodeType && obj.nodeName && obj.cloneNode) {
                        out[key] = obj[key];
                    }
                    else out[key] = extend(out[key], obj[key]);
                }
                else out[key] = obj[key];
            }
        }
    }
    return out;
}

However, I think that's a somewhat unusual solution; personally if I encountered a function called extend() I'd expect it to either deep-copy or shallow-copy without special exceptions. So I'd recommend using jQuery objects or some similar wrapper object rather than the DOM nodes themselves.

UPDATE

As mentioned above, the extend() method has another issue that causes it not to clone jQuery objects properly. The issue is that it doesn't preserve the object's prototype, but rather just starts with an empty object {}. You could fix that by using Object.create(), but it still would take extra work to fix some other issues with it (see comments below). So I recommend just using an existing 3rd party cloning function instead.

For example, you could use the cloneDeepWith function from lodash:

settings = _.cloneDeepWith(this.defaults, options);

(Note: Apparently lodash just copies a reference for DOM nodes and deep-clones everything else, so in that respect it works like the modified version of extend() I wrote above. So it might be a convenient option for you.)

Or jQuery's extend() method using the deep option:

settings = $.extend(true /* setting the first argument to true makes it deep copy */,
    {}, this.defaults, options);

I also wrote my own deepCopy() function which is quite robust...it's party of my simpleOO library which will become irrelevant over time now that Javascript has classes, but here's the link to the documentation on the deepCopy method if you're interested:

https://github.com/mbrowne/simpleoo.js/wiki/Additional-Examples#deep-copying-cloning-objects

(My deepCopy function can also be used on its own; see the second example in my answer here: https://stackoverflow.com/a/13333781/560114.)

Community
  • 1
  • 1
Matt Browne
  • 12,169
  • 4
  • 59
  • 75
  • thanks for your detailed answer. however i find that in `Object.assign()` it changes the value of original object (also stated in MDN). I went with the "dont deep copy DOM" way and is doing fine so far, but I modified to use it like `HTML(\w{1,}Element/i.test(obj[key].constructor)`. – yqlim Mar 03 '17 at 04:14
  • Right, that's what I meant above when I said `Object.assign` only makes a shallow copy. It would be a quick fix that would only work if your component doesn't mutate the `options`. Your modified version should work fine for HTML elements. It wouldn't catch other DOM nodes like text nodes and comments, but you may not need that. – Matt Browne Mar 03 '17 at 12:48
  • But if your `extend` method is something you want to reuse in the future, I strongly suggest making it at least capable of handling non-enumerable properties. A lot of libraries as well as ES6 classes make methods non-enumerable (that was the issue with jQuery objects using your `extend`) so sooner or later you would have issues with it. Also it doesn't preserve the prototype of the object and in the case of cyclic references (object `a` points to `b` and `b` also points to `a`) it would cause an infinite loop. – Matt Browne Mar 03 '17 at 12:52
  • I wrote my `deepCopy` function a while ago and realized it doesn't copy non-enumerable properties either. And actually my comments about jQuery might not have been accurate. In any case, to make your `extend` function better handle native objects in general, you could do what lodash does...I took a look at their source code and it looks like they only deep-copy arrays and user-defined objects. You can check for user-defined objects using `Object.prototype.toString.call(obj[key]) === 'object'` – Matt Browne Mar 03 '17 at 13:23
  • Ah, I see...sorry to write a novel here with all these comments, but to correct what I said earlier, the issue with jQuery objects (and many other objects) in your `extend` function is mainly that it doesn't preserve the prototype and it only copies own properties, not inherited properties on the prototype. So at the very least you should use `Object.create` to create the new objects; see my `deepCopy` function for an example. And thank you for inspiring me to write a new version of `deepCopy` to better accommodate situations like yours...I'll do that soon. – Matt Browne Mar 03 '17 at 14:05