angular.extend does not copy getters/setters

[mschwartz]

var o = angular.extend( {}, { 
  get x() { 
    return 'x';
  } 
});

resulting o object does not have the get x() method, it has 'x'

[mschwartz]

See a proper version here:

https://github.com/decafjs/decaf/blob/master/builtins/decaf.js#L17

extend() method

[mschwartz]

Same bug exists in jQuery.

[mschwartz]

While you're looking at it, you may want to consider ES6 Proxy support, too

[caitp]

Angular doesn't need to detect getters/setters as it currently doesn't use getters/setters anywhere yet. This also applies to ES6 proxies, which don't really exist in very many browsers (v8 used to, as far as I can recall, implement Mozilla's old Proxy API, but this is no longer the case, if it ever was and I'm not misremembering).

Personally, I'd love to add these features, but I don't think we'd have an actual use for them in core at this time, and it would hurt execution speed even more.

If you need features like this, why not implement them in your application code?

[mschwartz]

The method does not perform its function as advertised. It does not truly copy properties from the src objects to the dst.

The jQuery implementation is many lines longer than the angular.extend() implementation. They implement deep copy and all sorts of bounds checking and even calls itself recursively.

ES6 Proxy has been in Firefox since version 24.

jQuery.extend = jQuery.fn.extend = function() {
    var options, name, src, copy, copyIsArray, clone,
        target = arguments[0] || {},
        i = 1,
        length = arguments.length,
        deep = false;

    // Handle a deep copy situation
    if ( typeof target === "boolean" ) {
        deep = target;

        // Skip the boolean and the target
        target = arguments[ i ] || {};
        i++;
    }

    // Handle case when target is a string or something (possible in deep copy)
    if ( typeof target !== "object" && !jQuery.isFunction(target) ) {
        target = {};
    }

    // Extend jQuery itself if only one argument is passed
    if ( i === length ) {
        target = this;
        i--;
    }

    for ( ; i < length; i++ ) {
        // Only deal with non-null/undefined values
        if ( (options = arguments[ i ]) != null ) {
            // Extend the base object
            for ( name in options ) {
                src = target[ name ];
                copy = options[ name ];

                // Prevent never-ending loop
                if ( target === copy ) {
                    continue;
                }

                // Recurse if we're merging plain objects or arrays
                if ( deep && copy && ( jQuery.isPlainObject(copy) ||
                    (copyIsArray = jQuery.isArray(copy)) ) ) {

                    if ( copyIsArray ) {
                        copyIsArray = false;
                        clone = src && jQuery.isArray(src) ? src : [];

                    } else {
                        clone = src && jQuery.isPlainObject(src) ? src : {};
                    }

                    // Never move original objects, clone them
                    target[ name ] = jQuery.extend( deep, clone, copy );

                // Don't bring in undefined values
                } else if ( copy !== undefined ) {
                    target[ name ] = copy;
                }
            }
        }
    }

    // Return the modified object
    return target;
};

[mschwartz]

Thanks!

[trisys3]

extend actually does work with getters and setters, you just have to make them enumerable. One way is to use defineProperty:

Object.defineProperty(obj, 'prop', {
    get: function() {
        return this._prop;
    },
    set: function(newProp) {
        this._prop = newProp;
    },
    enumerable: true
});

Now, if you use angular.extend to extend the obj object, the setter/getter will come along for the ride:

var newObj = angular.extend({}, obj);

The newObj object will now have everything obj had, including prop.

I'm not sure if there is a way to make the extend function work without loops, which is what you'd have to do to support non-enumerable getters and setters, but if you have getters and setters you want to merge with another object, you probably want them to be enumerable anyway.

[trisys3]

Actually, if you make a getter or setter enumerable when defining the property with defineProperty, which is one of the only ways to add a getter or setter after the object is created, it is merged into the other object. However, if you create the first object with a getter or setter, it is not copied over, even though it is already enumerable (according to Firebug). I am not sure why this is, as in both cases the object is not writable and with defineProperty the getter/setter is not configurable but when creating with a getter/setter it is configurable.

[mschwartz]

Here's how it's done right.

https://github.com/decafjs/decaf/blob/master/builtins/decaf.js#L47

On Wed, Aug 19, 2015 at 3:13 PM, Sean Moran notifications@github.com
wrote:

Actually, if you make a getter or setter enumerable when defining the
property with defineProperty, which is one of the only ways to add a
getter or setter after the object is created, it is merged into the other
object. However, if you create the first object with a getter or setter, it
is not copied over. I am not sure why this is, as in both cases the object
is not writable and with defineProperty the getter/setter is not
configurable but when creating with a getter/setter it is configurable.

—
Reply to this email directly or view it on GitHub
#8573 (comment)
.

[trisys3]

I was not proposing an alternative to angular.extend, I was mentioning a way to use angular.extend with getters and setters without changes to angular. I'm sure your method is fine, and I may try to use it instead of Angular's, but most people, including me at work, will wait until Angular bundles it or something like it into core.

[mschwartz]

defineProperty() isn't the only way to define a getter or setter.

var o = { _x: 10, get x() { return this._x; } };

I'm reasonably sure you don't want angular.extend() to copy this._x (10)
into the extended object, but rather the get() function.

IMO

On Wed, Aug 19, 2015 at 4:05 PM, Sean Moran notifications@github.com
wrote:

I was not proposing an alternative to angular.extend, I was mentioning a
way to use angular.extend with getters and setters without changes to
angular. Sorry if I was not more clear.

—
Reply to this email directly or view it on GitHub
#8573 (comment)
.

[trisys3]

I am aware that defineProperty is not the only way to define a getter or setter. The Mozilla network's site mentions it as the only way to define a getter/setter after the object is already created (toward the bottom of the page). More importantly, however, defineProperty allows you to change the enumerability, which was key to the method I mentioned to successfully extend an object with getters &/or setters.

Then again, when a getter/setter is created at the time the object is created, it is already enumerable, so I don't know why these getters/setters can not be extended by default, but they can't.

[mschwartz]

The angular implementation of extend() is calling the getter and/or setter
of the source object instead of detecting it is a getter/setter and
creating a getter/setter function on the destination. The enumeration is
finding the member as a variable (e.g. return value from the get method).

A true copy means it copies the members: variables and functions. A getter
or setter is a function.

Getters and setters are pretty slick. If you want to do a true extend(),
you're going to have to call angular.extend() then call defineProperty() or
equivalent as many times as you have setters and getters and you're going
to have to know the name of each, etc.

The key to making extend() work is in that decafjs code.

On Wed, Aug 19, 2015 at 5:08 PM, Sean Moran notifications@github.com
wrote:

I am aware that defineProperty is not the only way to define a getter or
setter. The Mozilla network's site mentions it as the only way to define
one after the object is already created. Additionally, defineProperty
allows you to change the enumerability, which was key to the method I
mentioned to successfully extend an object with getters &/or setters. Then
again, when a getter/setter is created when the object is, it is already
enumerable, so I don't know why these getters/setters can not be extended,
but they can't.

—
Reply to this email directly or view it on GitHub
#8573 (comment)
.

[lgalfaso]

Angular exposes some of the helper functions that it uses internally. This is the case for `extend`, `copy` and many others. There are other libraries that specialize in these functions, keep their focus is there and can do a better job. Eg, angular.copy does not handle many of the browser specific quirks with native objects and probably will never do. It is not in the best interest of most users to make these helper functions big nor slow, as these are used internally and any change in that direction can have a direct impact in download size and performance. At the same time, apps that need the most accurate version, should be better served with other libraries. Now, anybody is welcome to create a PR that adds support for getters/setters. If the changes are small and the performance is about the same, then it should get merge without any issues. If the change makes some function big or slow, then it will need an excellent use case or a large community supporting this trade off (in these later cases, it should get eventually solved). Anybody who is interested in getting this fixed, create a PR and let's move the discussion there.

[StephanBijzitter]

So why does angular provide an extend function if it does not work as advertised? You're telling us to use different libraries, fine, but then remove the function from your own library and depend on those others libraries as dependencies.

[trisys3]

I think what @Igalfaso is trying to say is that angular tries to make its built-in functions as small and fast as it needs for its own internal purposes. For example, it does not need its extend function to be as accurate as the one that decafjs defines, because internally none of the things it is doing need an extremely accurate extend function. If angular depended on a library like decafjs, that would make it bigger & slower, in addition to functions like extend, from decafjs, being bigger and slower than it needs. If the end developer needs a more accurate function for extending objects, and does not mind the performance penalty, he/she can use decafjs.

[vadimcoder]

Related to #5085

[egidiocs]

@mschwartz thank you for this: var o = { _x: 10, get x() { return this._x; } };

[wesleycho]

Given the Angular team's general stance about the utility functions it provides, would it make sense to close this issue or maybe have a doc update that warns that it should not be relied upon as a robust solution for all conditions?

[trisys3]

I'm really just a user, but I for one would have appreciated a doc reference pre-empting my comment here.

[gkalpak]

@wesleycho is right. Closing as won't fix.
PRs updating the docs are always welcome, if anyone feels like it 😃