433351 - Implement Object.extend

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect, P1)

Description

[Robert Sayre]

Lots of JS libraries have this.

Comment 1

[Damon Sicore (:damons)]

getting this on the 1.9.1 triage queue.

Comment 2

[John Resig]

Attachment: Pure-JavaScript Object.extend

This is a pure-JavaScript version of Object.extend that I'm proposing. It includes support for getters and setters and does a simple loop over the object properties to bring them on to the base object.

Comment 3

[Brian Crowder]

jresig: a couple thoughts on your Pure-JavaScript version:

* As mentioned on IRC, I expected a hasOwnProperty check in here somewhere.
* In the jQuery implementation, you check to see if the obj being copied from is of type "object" or "function", and I think this restriction is sane.

Comment 4

[Brian Crowder]

jsresig:  Also, should I be overwriting previously set values on orig, or not?

iow, Object.extend({a:3}, {a:5}) does what?

My inclination is -not- to overwrite them.

Comment 5

[John Resig]

> * As mentioned on IRC, I expected a hasOwnProperty check in here somewhere.

The version that I passed to you on IRC has one:
  http://ejohn.org/files/object-extend.js

I'm working to try and include more test cases.

> * In the jQuery implementation, you check to see if the obj being copied from
> is of type "object" or "function", and I think this restriction is sane.

Yeah, that's probably a good thing to test for - I agree. I'll add that to my working copy.

> My inclination is -not- to overwrite them.

All of the other implementations override by default. The only one that provides an option to toggle that behavior is Yahoo UI's (as an extra argument).

Comment 6

[Brian Crowder]

Attachment: v1

Here's a stab at an implementation of this for spidermonkey.  I haven't even begun to try it against John's test-suite yet.  That's next.

Comment 7

[Brian Crowder]

Okay, my version seems to pass all 85 of the samples included in your suite, John.

Comment 8

[Brian Crowder]

Comment on attachment 330306 [details] [diff] [review]
v1

I'll ask for Brendan's review once you've looked it over.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 330306 [details] [diff] [review]
v1

>+++ mozilla/js/src/jsobj.cpp	2008-07-18 15:02:02.000000000 -0700
>+    JS_SET_RVAL(cx, vp, JS_ARGV(cx, vp)[0]);

It seems worth it to move the JS_ARGV computation into a local jsval *argv.

>+    orig = js_ValueToNonNullObject(cx, JS_ARGV(cx, vp)[0]);
>+    if (!orig) {
>+        return JS_TRUE;
>+    }

Nit: overbraced if statement.

Non-nit: js_ValueToNonNullObject sets an exception if it fails. You need to either JS_ClearPendingException or return JS_FALSE here. The latter makes more sense to me.

>+    for (i = 1; i < argc; i++) {
>+        jsval iter_state;
>+
>+        obj = js_ValueToNonNullObject(cx, JS_ARGV(cx, vp)[i]);
>+        if (!obj)
>+            continue;

Need to throw here or clear the pending exception.

>+            if (!OBJ_LOOKUP_PROPERTY(cx, obj, id, &obj2, &prop))
>+                return JS_FALSE;

Need to ensure that you call into the enumerator with JSENUMERATE_DESTROY here and under js_DefineNativeProperty.

>+
>+            if (obj != obj2) {   /* Don't copy inherited props */
>+                OBJ_DROP_PROPERTY(cx, obj2, prop);
>+                continue;
>+            }

The enumeration API is only supposed to return direct properties of the given object, so the above check shouldn't be necessary.

crowder and I talked on IRC about using the JS_Enumerate API versus the OBJ_ENUMERATE API directly. I'll let brendan make the final call -- crowder argued that he didn't need to enumerate the properties twice.

Comment 10

[Brian Crowder]

Attachment: v2

I think we don't want to throw for primitive "orig" (see jresig's original JS implementation), so I've reworked that a bit.  I'll propagate an exception if js_ValueToNonNullObject throws for non-null objects, though.  This version does the same as I loop through the arguments, as well.

Did a DESTROY under the GET/SET pair at the bottom of the loop, also, where I also return JS_FALSE.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 332430 [details] [diff] [review]
v2

Looks OK to me. The API around primitive values seems wonky to me, but I'll let jresig and the rest of y'all fight it out.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 332430 [details] [diff] [review]
v2

After talking with brendan, we decided that the caller of the OBJ_ENUMERATE api is responsible for rooting the iter_state passed down. This is done "for free" in the for-in case, but anybody else needs to use a tvr or something.

Comment 13

[Brian Crowder]

Mass unowning JS bugs...  I'm not likely to be able to move these forward.

Comment 14

[Jeff Walden [:Waldo]]

I think we've moved to doing this through the ECMAScript mailing list and working group these days, so I don't think we're doing this unless ECMA picks it up first.