Prototype

Comments and changes to this ticket

• 

  fearphage August 20th, 2008 @ 11:53 PM

  Legible performance numbers * Safari 3.1.2 (windows) - 1484 / 391 * FF 3.0.1 - 1352 / 755 * IE6 - 7031 / 1891 * Opera 9.52 - 4406 / 2375

• 

  John-David Dalton August 21st, 2008 @ 12:15 AM

  +10 thank you for posting benchmarks

  Safari 2.0 has an issue with passing a function as the second param. It will try to convert it to a string.

  We could do

  
  
  String.prototype.camelize = (function() {
  
    'a'.replace('a', function() { return 'b' }) === 'b' ?
      return function() {
        this.replace(/\-+(.)?/g, function(match, char) {
          return (char || '').toUpperCase();
        })
      } :
      return function() {
        var parts = this.split('-'), len = parts.length;
        if (len == 1) return parts[0];
  
        var camelized = this.charAt(0) == '-'
          ? parts[0].charAt(0).toUpperCase() + parts[0].substring(1)
          : parts[0];
  
        for (var i = 1; i < len; i++)
          camelized += parts[i].charAt(0).toUpperCase() + parts[i].substring(1);
  
        return camelized;
      };
  })();
  

• 

  John-David Dalton August 21st, 2008 @ 12:14 AM

  • → Tag changed from “enhancement performance string” to “benchmarked enhancement needs_patch needs_tests performance string”
  • → Milestone changed from “” to “1.6.1”
  • → State changed from “new” to “enhancement”
• 

  Juriy Zaytsev August 21st, 2008 @ 05:49 AM

  Safari 2 should be killed violently ; )

  All the workarounds for a lack of hasOwnProperty and function-based replace are becoming ridiculous.

• 

  John-David Dalton August 21st, 2008 @ 06:37 AM

  Ya I am starting to think the same thing.

• 

  fearphage August 21st, 2008 @ 09:47 AM

  For performance reasons, I would suggest that checked it once (when defining camelize) and not every single time it was used.

  Alternatively, you could sniff safari 2.x and fix String#replace:

  
  (function(replace){
    String.prototype.replace = function(pattern, replacement){
      // replacement is not function, use built-in
      if (typeof replacement != 'function')
        return replace.apply(this, arguments);
  
      var str = '' + this;
      // pattern string is not RegExp
      if (!(pattern instanceof RegExp)) {
        var idx = str.indexOf(pattern);
        return (idx == -1 ?
          str :
          replace.apply(str, [pattern, replacement(pattern, idx, str)]);
        );
      }
  
      var reg = pattern, result = [], lastidx = reg.lastIndex, re;
      while ((re = reg.exec(str)) != null) {
        var idx  = re.index, args = re.concat(idx, str);
        result.push(str.slice(lastidx, idx), replacement.apply(null, args).toString());
        if (!reg.global) {
          lastidx += RegExp.lastMatch.length;
          break;
        }
        else {
          lastidx = reg.lastIndex;
        }
      }
      result.push(str.slice(lastidx));
      return result.join('');
    }
  })(String.prototype.replace);
  

  This is old code (sanitized by me) that I found here.

• 

  Tobie Langel August 22nd, 2008 @ 01:33 AM

  Premise:

  String#camelize is insanely long for something that can be completed in one line

  Initial code:

  
  function() {
    var parts = this.split('-'), len = parts.length;
    if (len == 1) return parts[0];
  
    var camelized = this.charAt(0) == '-'
      ? parts[0].charAt(0).toUpperCase() + parts[0].substring(1)
      : parts[0];
  
    for (var i = 1; i < len; i++)
      camelized += parts[i].charAt(0).toUpperCase() + parts[i].substring(1);
  
    return camelized;
  }
  
  

  What we end up with:

  
  (function(replace){
    String.prototype.replace = function(pattern, replacement) {
      // replacement is not function, use built-in
      if (typeof replacement != 'function')
        return replace.apply(this, arguments);
  
      var str = '' + this;
      // pattern string is not RegExp
      if (!(pattern instanceof RegExp)) {
        var idx = str.indexOf(pattern);
        return (idx == -1 ?
          str :
          replace.apply(str, [pattern, replacement(pattern, idx, str)]);
        );
      }
  
      var reg = pattern, result = [], lastidx = reg.lastIndex, re;
      while ((re = reg.exec(str)) != null) {
        var idx  = re.index, args = re.concat(idx, str);
        result.push(str.slice(lastidx, idx), replacement.apply(null, args).toString());
        if (!reg.global) {
          lastidx += RegExp.lastMatch.length;
          break;
        }
        else {
          lastidx = reg.lastIndex;
        }
      }
      result.push(str.slice(lastidx));
      return result.join('');
    }
  })(String.prototype.replace);
  
  function() {
    return this.replace(/\-+(.)?/g, function(match, char) { return (char || '').toUpperCase() });
  }
  

  Am I the only one to feel weird about this ?

• 

  Tobie Langel August 22nd, 2008 @ 01:37 AM

  • → Tag changed from “benchmarked enhancement needs_patch needs_tests performance string” to “needs_benchmarks needs_patch needs_tests performance string”

  More seriously though, this is an optimization that depends on a new feature (fixing String#replace for Safari). It should be marked as such and a new ticket should be opened for String#replace fix. Also benchmarks should be made available (i.e. provided using the benchmark method in the unit test suite and commented out not to impact the speed of the tests.

• 

  Juriy Zaytsev August 22nd, 2008 @ 03:05 AM

  Tobie,

  I would like to propose dropping support for Safari 2.0.0 - 2.0.2. Those 3 minor releases bring havoc upon us.

  The #camelize situation shows how bizarre things are (and how much better they can be).

  Can we consider this for 1.6.1?

• 

  fearphage August 22nd, 2008 @ 07:02 AM

  Am I the only one to feel weird about this ? The difference being that the old camelize can be accomplished with much more concise code. Fixing replace is not as simplistic.

  Also benchmarks should be made available (i.e. provided using the benchmark method in the unit test suite and commented out not to impact the speed of the tests. It is very hard to find someone with Safari 2. I asked around to see if I could acquire some benchmarks. Would it be wrong for users of old stuff to take a performance hit?

  I would like to propose dropping support for Safari 2.0.0 - 2.0.2. This seems like a reasonable option as well.

  Do any of you know anyone with access to Safari 2? I think benchmarking Safari 2 will be the most difficult part of this fix.

• 

  fearphage August 22nd, 2008 @ 07:24 AM

  Is there a place where browser support is listed? Google and I can not find it.

  Side note: I hate the new wiki formatting stuff and it sucks not being able to edit comments.

• 

  John-David Dalton August 22nd, 2008 @ 07:43 AM

  I have Safari 2.0 native but you can also download it here: http://michelf.com/projects/multi-safari/

  If you produce the benchmarks I can test it. I think its simpler to drop support for it and release an optional adapter if a dev requests support. I believe Apple even dropped support for 2.0

• 

  fearphage August 22nd, 2008 @ 08:10 AM

  Supported versions (for reference):

  jQuery - Safari 2.0.2+ YUI - Safari 3.1+ Rico - Safari 2.0.3 Dojo - Latest Safari (currently 3.0.x), 2.0 support was dropped upon the release of Leopard Ext JS - Safari 3+ qooxdoo - Safari 3.0 GWT - Safari 2.0

• 

  fearphage August 22nd, 2008 @ 08:15 AM

  more...

  Mootools - Safari 3+ MochiKit - 2.0.2

• 

  Tobie Langel August 22nd, 2008 @ 10:16 AM

  I'm not too concerned about performance issues in Safari < 3. However, we should still aim to support it as much as possible, or at least, not break it on purpose.

  I'd definitely opt for modifying String#replace where necessary.

  I'd avoid the instanceof Regexp part, though, in favor of a duck-typing to avoid cross frame issues.

• 

  fearphage August 22nd, 2008 @ 11:17 AM

  Filed String#replace for Safari 2 as bug #300

  I didn't incorporate the duck typing.