Closed Bug 433351 Opened 12 years ago Closed 9 years ago

Implement Object.extend


(Core :: JavaScript Engine, defect, P1)






(Reporter: sayrer, Unassigned)



(2 files, 1 obsolete file)

Lots of JS libraries have this.
getting this on the 1.9.1 triage queue.
Flags: blocking1.9.1?
Priority: -- → P1
Flags: wanted1.9.1+
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Hardware: PC → All
Version: unspecified → Trunk
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.
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.
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.
> * As mentioned on IRC, I expected a hasOwnProperty check in here somewhere.

The version that I passed to you on IRC has one:

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).
Attached patch v1 (obsolete) — Splinter Review
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.
Okay, my version seems to pass all 85 of the samples included in your suite, John.
Comment on attachment 330306 [details] [diff] [review]

I'll ask for Brendan's review once you've looked it over.
Attachment #330306 - Flags: review?(mrbkap)
Comment on attachment 330306 [details] [diff] [review]

>+++ 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.
Attachment #330306 - Flags: review?(mrbkap) → review-
Attached patch v2Splinter Review
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.
Attachment #330306 - Attachment is obsolete: true
Attachment #332430 - Flags: review?(mrbkap)
Comment on attachment 332430 [details] [diff] [review]

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.
Attachment #332430 - Flags: review?(mrbkap) → review+
Comment on attachment 332430 [details] [diff] [review]

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.
Attachment #332430 - Flags: review+ → review-
Mass unowning JS bugs...  I'm not likely to be able to move these forward.
Assignee: crowder → general
Assignee: general → jwalden+bmo
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.
Assignee: jwalden+bmo → general
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.