#382 ✓resolved
Samuel Lebeau

Subclass doesn't inherit superclass `toString` and `valueOf`properties on IE

Reported by Samuel Lebeau | October 8th, 2008 @ 08:58 PM | in 1.6.1

On Internet Explorer,


Class.create(AncestorClass, methods);

will add


methods.toString

even if it's


Object.prototype.toString

and


AncestorClass.prototype

redefines it.

Attached patch shows a test case and fixes this bug.

Comments and changes to this ticket

  • Samuel Lebeau

    Samuel Lebeau October 8th, 2008 @ 09:03 PM

    • Assigned user set to “Sam Stephenson”
  • Tobie Langel

    Tobie Langel October 9th, 2008 @ 02:06 PM

    • Assigned user changed from “Sam Stephenson” to “Tobie Langel”
    • State changed from “new” to “bug”
    • Milestone set to 1.6.1
    • Tag changed from bug, class, patched to class, patched
  • Samuel Lebeau
  • Juriy Zaytsev

    Juriy Zaytsev February 23rd, 2009 @ 12:09 AM

    • Tag changed from class, patched to patched, tested
    • Assigned user changed from “Tobie Langel” to “Juriy Zaytsev”

    @Samuel

    If you don't mind, I'll rewrite this patch to avoid creation of object and Object.keys() call every time addMethods is called. It seems unnecessary; we can test for "DontEnum" bug at load time and then only check corresponding boolean at run time.

    While at it, we should take care of other Object.prototype.* methods (that IE doesn't enumerate over) for completeness, and since it doesn't cost us much (size/speed)

  • Samuel Lebeau

    Samuel Lebeau February 23rd, 2009 @ 12:30 AM

    Sure !

    But shouldn't Object.keys and Object.values be fixed in first place, and have addMethods delegate to Object.keys ?

  • Juriy Zaytsev

    Juriy Zaytsev February 23rd, 2009 @ 12:33 AM

    Yes it should!

    We had a ticket for this some time ago (which, IIRC, was to take care of exactly this case).

    Thanks for reminding : )

  • Tobie Langel

    Tobie Langel February 23rd, 2009 @ 12:34 AM

    Object.keys should map to ES 3.1 specs imho.

  • Juriy Zaytsev

    Juriy Zaytsev February 23rd, 2009 @ 12:39 AM

    Tobie,

    Mapping to ES3.1 would involve adding second argument for sorting and throwing error if first argument is not an Object. That might be a bit obtrusive for 1.6.0.4 but I would be in favor of doing this in 1.6.1.

    Fixing DontEnum bug in Object.keys would not affect ES3.1 Object.keys in any way (since it's IE that's not compliant and fails to enumerate over eligible properties - those with DontEnum flag set to false)

  • Juriy Zaytsev

    Juriy Zaytsev February 23rd, 2009 @ 12:41 AM

    @Samuel

    Here's the old ticket, but I'll write a new patch anyway http://prototype.lighthouseapp.c...

  • Juriy Zaytsev

    Juriy Zaytsev February 23rd, 2009 @ 12:43 AM

    • Tag changed from patched, tested to needs_patch, needs_tests
    • Title changed from “Class.create bug with toString on IE” to “Object.keys should handle JScript's DontEnum bug”
  • Juriy Zaytsev

    Juriy Zaytsev February 23rd, 2009 @ 02:05 AM

    • Tag changed from needs_patch, needs_tests to patched, tested
    • Assigned user changed from “Juriy Zaytsev” to “Andrew Dupont”
  • Tobie Langel

    Tobie Langel February 23rd, 2009 @ 09:05 AM

    Shouldn't this fix actually be in an @Object.forEach@, rather than in Object.keys ?

  • Samuel Lebeau

    Samuel Lebeau February 23rd, 2009 @ 02:47 PM

    I think so.

    I'm still wondering :

    • if forEach(object, iterator) should be public.
    • if iterator should be called with two arguments (property and corresponding value), which isn't really consistent with Hash API regarding enumeration, but can avoid performance issues.
    • If Object.extend should use it too, which would imply a real performance impact, I guess...
  • Andrew Dupont

    Andrew Dupont February 24th, 2009 @ 03:06 AM

    • Tag changed from patched, tested to discuss, patched, tested

    Can we come to consensus on these issues in the next week or so, or shall we defer this fix to the next milestone?

  • Samuel Lebeau

    Samuel Lebeau February 24th, 2009 @ 03:48 PM

    Here is my take :

    Added Object.forEach(object, iterator(property, value)), both Object.keys and Object.values are delegating to it. Object.extend delegates to it only if DontEnum is buggy.

    What do you guys think ?

  • Tobie Langel

    Tobie Langel February 24th, 2009 @ 06:01 PM

    I'd suggest not exposing the API for now.

    Could we decide on a pattern for pseudo constants and keep to it? I'd strongly advocate using FOO_BAR over FooBar.

    Also, could we handle troublesome browsers with a single runtime check rather than on each method call?

  • Samuel Lebeau
  • Samuel Lebeau

    Samuel Lebeau February 25th, 2009 @ 12:30 AM

    Here is another attempt.

    I removed forEach and introduces a private iterateOverDontEnumProperties method, which is called by extend, keys and values only if DONT_ENUM_BUGGY (consistent with CONCAT_ARGUMENTS_BUGGY) is true.

    This may not be very DRY, but I think this is the best performance / code size compromise, and it only costs a boolean check on non-buggy platforms.

  • Tobie Langel

    Tobie Langel February 25th, 2009 @ 01:10 PM

    Can you upload your patch, or best, link to a pull request ?

  • Juriy Zaytsev

    Juriy Zaytsev February 25th, 2009 @ 03:01 PM

    Last patch looks good, but why for..in instead of while?

    I'm also in favor of FOO_BAR.

  • Tobie Langel

    Tobie Langel February 25th, 2009 @ 04:02 PM

    I'd argue that, although while... is currently slightly faster at
    iterating over the members of an array than for... is, this might
    not be the case in future implementations. We should follow whatever
    the specs mandates as the "correct" way of doing so in order to
    benefit from further speed improvements.

    On Feb 25, 2009, at 15:01, Lighthouse no-reply@lighthouseapp.com
    wrote:

  • Juriy Zaytsev

    Juriy Zaytsev February 25th, 2009 @ 04:53 PM

    Tobie, the specs mandates nothing regarding order of iteration so I don't see why we wouldn't want to use faster while.

  • Tobie Langel

    Tobie Langel February 25th, 2009 @ 06:45 PM

    I thought so. Whatever we do, I think we should keep consistency throughout the src code.

  • Juriy Zaytsev

    Juriy Zaytsev February 25th, 2009 @ 07:01 PM

    I'm all for consistency too, of course, but using faster loop in critical parts seems like a reasonable thing to do. The same way, it's a good idea to fork some methods at load time to eliminate extra run time costs.

  • Tobie Langel

    Tobie Langel February 25th, 2009 @ 07:09 PM

    It's a good idea to fork all methods at runtime.

  • Juriy Zaytsev

    Juriy Zaytsev February 25th, 2009 @ 08:30 PM

    Sometimes forking at runtime is slow and unnecessary. Why would you want that?

    Btw, it looks like ES3.1 removed sort parameter. I suppose Object.keys would now look like:

    
    /*
    
    15.2.3.14 Object.keys ( O ) 
    
      When the keys function is called with argument O, the following steps are taken: 
      1. If the Type(O) is not Object, throw a TypeError exception. 
      2. Let array be the result of creating a new Object as if by the expression new Array() where 
        Array is the standard built-in constructor with that name. 
      3. For each own enumerable property of O, append the key string of the property to array. 
      4. Return array. 
    
    */
    
    (function(){
      function isPrimitive(o) {
        return o == null || /^(boolean|number|string)$/.test(typeof o);
      }
      var hasOwnProperty = Object.prototype.hasOwnProperty;
      function keys(o) {
        if (isPrimitive(o)) {
          throw TypeError('Object is required');
        }
        var arr = []; 
        for (var prop in o) {
          if (hasOwnProperty.call(o, prop)) {
            arr.push(prop);
          }
        }
        return arr;
      }
      Object.keys = keys;
    })();
    
  • Radoslav Stankov

    Radoslav Stankov February 26th, 2009 @ 12:42 AM

    Is there a particular reason to define a keys function first, and then assign the Object.key to it ? Or is just the because of the consistency ?

  • Tobie Langel

    Tobie Langel February 26th, 2009 @ 01:17 AM

    There are two reasons to do so:

    1. Have a named function rather than an anonymous one. Great for
      debugging.
    2. Avoid redefining a native method if it already exists:
    
    if (!('keys' in Object)) {
       Object.keys = keys;
    }
    
  • Tobie Langel

    Tobie Langel February 26th, 2009 @ 01:19 AM

    Sometimes forking at runtime is slow and unnecessary.

    When is that the case?

  • Radoslav Stankov

    Radoslav Stankov February 26th, 2009 @ 01:53 AM

    hm, 10x for the explanation :)

  • Tobie Langel

    Tobie Langel February 26th, 2009 @ 02:17 AM

    kangax: From what I understand from the specs, it seems like Type(O) could be defined as:

    
      function isObject(o) {
        return typeof o === 'object' && o !== null;
      }
    

    I suspect that your isPrimitive handles JScript edge cases with objects of type 'unknown' and the like. Am I correct in assuming that this was your intent ?

  • Juriy Zaytsev

    Juriy Zaytsev February 26th, 2009 @ 04:08 AM

    Tobie, my main intention was actually to match Function objects (or any objects for that matter).

    ES3.1 is vague at explaining what Type really is. Object.keys (15.2.3.14) says that when Type(O) is not Object throw type error. What is Type, then?

    5.2 says:

    
    Type(x) is used as shorthand for "the type of x"
    

    What's type of x, then?

    Type (4.3.1) says:

    
    A type is a set of data values as defined in section 8 of this specification.
    

    Section 8, in its turn, lists - The Undefined Type, The Null Type, ... , and finally The Object Type (8.6)

    8.6 says:

    
    An Object is a collection of properties. Each property is either a named data property, a named accessor property, or an internal property.
    

    There are no "Function objects" or "RegExp objects", etc. It seems that section 8 lists primitive types, object type and internal types, such as Reference.

    By Type(Object), I think specs really mean - any object - as a generic collection of properties - or, in other words, any non-primitive value.

    It might be a good idea to bring this up on es-discuss (as other people might run into similar difficulties when implementing, say, ES3.1 object extensions in ES3 : ))

  • Andrew Dupont

    Andrew Dupont February 26th, 2009 @ 04:59 AM

    I tend to defer to Juriy on these issues, since he has read the spec more thoroughly than I have. ;-)

    Can we do some quick profiling in IE, comparing the existing Object.extend with a "fixed" version that uses Object.keys internally? It might be worth duplicating the Object.keys logic inside of Object.extend so that we don't have to loop through the object twice.

  • Tobie Langel

    Tobie Langel February 26th, 2009 @ 09:26 AM

    Right, Kangax. I totally forgot about function objects. And agreed
    regarding the necessity of specifying more clearly the meaning of Type
    in ES 3.1. Care to start a thread over there, or shall I?

  • Juriy Zaytsev

    Juriy Zaytsev February 26th, 2009 @ 02:46 PM

    YUI's last commit just introduced similar static Object.* extensions. I love the use of second argument as a "switch" - must be really speedy (no need to create different functions for keys/values/etc.), although they forgot to take care of DontEnum bug http://github.com/yui/yui3/commi...

    Tobie, I'm somewhat busy this whole week, so feel free to start a thread over at es-discuss. Regarding forking at load time, here's one (albeit a bit of an edge case) example - http://groups.google.com/group/c...

    Andrew, I'll make perf. tests once I get a chance, maybe also compare to YUI's way, which looks very promising.

  • Samuel Lebeau

    Samuel Lebeau February 26th, 2009 @ 03:57 PM

    Guys,

    The original bug of this ticket was : on platform with the DontEnum bug, due to a wrong DontEnum fix, a subclass will never inherit its superclass toString or valueOf methods, but will end up with Object.prototype ones.

    I think this is a serious issue which should be fixed ASAP.

    Now we're discussing how to implement an ES 3.1 compatible Object.keys, which imply throwing an error on incompatible objects types (not very Prototypesque) and maybe simulating Object.prototype.hasOwnProperty.

    Maybe a simple compromise, in a first time, would be just to fix Object.keys, Object.values and Object.extend so they can enumerate over DontEnum properties on buggy platforms.

    Having a simple guard like if (object == null) return; is sufficient to avoid errors, and keep the actual Object.keys(null) == [] behavior.

  • Tobie Langel

    Tobie Langel February 26th, 2009 @ 05:18 PM

    Good point, Samuel. IMHO, ES 3.1 compliance is a goal for am upcoming
    version of Prototype, not the next bug-fix release. It's worth
    discussing, though, but should probably have waranted a different
    thread.

  • Andrew Dupont

    Andrew Dupont February 28th, 2009 @ 11:35 AM

    • State changed from “bug” to “resolved”

    Fixed in http://github.com/sstephenson/pr...

    Let's open up a separate ticket for the Object.keys thing.

  • Juriy Zaytsev

    Juriy Zaytsev March 7th, 2009 @ 11:37 PM

    I finally asked about Type on es-discuss.

    https://mail.mozilla.org/piperma...

    In short, Object type implies an object in its generic sense - a collection of named properties, and not just something created via an Object constructor : )

    In ES3.1, Object is actually specified as one of the six language types - object, undefined, null, string, boolean and number, contrary to specification types like Reference, List, etc. See Allen Wirfs-Brock's explanation.

  • Tobie Langel

    Tobie Langel July 24th, 2009 @ 03:39 AM

    • Tag changed from discuss, patched, tested to needs:discussion, patched

    [not-tagged:"discuss" tagged:"needs:discussion" bulk edit command]

  • Samuel Lebeau

    Samuel Lebeau August 1st, 2009 @ 07:20 AM

    Proposal to fix both Object.keys, Object.values and Object.extend in a performance-wise way : http://github.com/samleb/prototype/commit/28fa6b05aa3e9883822a40dda...

    As far as I know, JScript implements Object.prototype.hasOwnProperty so we don't need to simulate it, but maybe I'm missing something here...

  • Tobie Langel

    Tobie Langel August 1st, 2009 @ 05:24 PM

    Samuel, two things:

    1. could you please open a new thread for this or yet, better a github branch.
    2. imho, fork should happen at load time, not on function invocation.
  • Samuel Lebeau

    Samuel Lebeau August 26th, 2009 @ 12:56 AM

    It seems that I've been fooled by this ticket's title and should have posted my patch on #382.

    This ticket was initially reporting a nasty bug in Class.create on IE that avoided a subclass to inherit its parent toString and valueOf (among others...).

    This bug is related to the DontEnum bug that affects Object.keys, Object.values and Object.extend but has been partially fixed (only toString and valueOf methods are inherited) in http://github.com/sstephenson/prototype/commit/47abfa68f0d6bd231e0c...

    I'm changing this ticket's title to avoid further confusion, and adding a link in #382 as a lot of what is discussed here is about DontEnum.

  • Samuel Lebeau

    Samuel Lebeau August 26th, 2009 @ 12:58 AM

    • Title changed from “Object.keys should handle JScript's DontEnum bug” to “Subclass doesn't inherit superclass `toString` and `valueOf`properties on IE”

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile ยป

The Prototype JavaScript library.

Shared Ticket Bins

Referenced by

Pages