millermedeiros / amd-utils

modular JavaScript utilities written in the AMD format
http://millermedeiros.github.com/amd-utils
142 stars 11 forks source link

avoid calling Array.prototype natives? #57

Closed millermedeiros closed 12 years ago

millermedeiros commented 12 years ago

That way it would be possible to call these methods over array-like objects (ie. jQuery collections) and we also wouldn't need to worry if behavior is exactly like the ES5 spec in every scenario* (will probably avoid headaches).

Another gain is that it would make the definition of some of the methods simpler (no need to check if native exists) and would avoid conflicts with other libs that modifies the natives as well.

I've seen some info about the native Array#forEach and other similar methods being slower than a custom implementation using while/for loops but that isn't the main reason for the change specially since I think perf will change in the future.

Affected methods:

PS: I won't drop support for sparse Arrays and will try to keep them as compatible as possible (for now keep the current implementation just avoid calling the natives).

jdalton commented 12 years ago

@millermedeiros I forked Underscore.js, creating Lo-Dash, and avoid native ES5 array methods. This gave Lo-Dash performance benefits for older browsers because the fallback didn't have to conform to spec (I don't support sparse arrays). It also allows Lo-Dash to work as a utility lib for code that is added to pages you don't control (widgets) because it won't break when loaded into pages that have something like an older Prototype.js adding bad native shims.

Another benefit of not supporting sparse arrays is it keeps the behavior consistent cross browser as IE < 9 will consider var a = [1, undefined, 3] as sparse, unlike other browsers.

petermichaux commented 12 years ago

In generic libraries, I generally avoid calling methods that might change when syntax is available. If the syntax version fits the needs then using it means I know it won't change like when someone redefines a method. That is good for the integrity of the intended functionality of the library. It is bad for the flexibility that a dynamic language allows.

I do use arr.push(el) rather than arr[arr.length] = el and so I'm not actually consistent with my approach.

jdalton commented 12 years ago

@millermedeiros Side note that your

jdalton commented 12 years ago

@millermedeiros For the ES5 native methods I do use I have a handy weak inference to detect if they have been shimmed, and if so, use my internal alternatives instead.

  var reNative = RegExp('^' +
    (Object.prototype.valueOf + '')
      .replace(/[.*+?^=!:${}()|[\]\/\\]/g, '\\$&')
      .replace(/valueOf|for [^\]]+/g, '.+?') + '$'
  );

  // usage
  if (reNative.test(Function.bind)) {
    /* do something */
  }
millermedeiros commented 12 years ago

@jdalton thanks for the comments and for spotting the bugs, fixed all of them on the this commit and also avoided using the natives altogether. Will wait a few days to merge just in case someone else has anything to say.

@petermichaux I will check other places on my code where I fallback to natives (like object/keys ), maybe I will drop them all. thx for the feedback.

millermedeiros commented 12 years ago

@jdalton BTW, I normalized the behavior of var a = [1, undefined, 3] between the browsers by using i in arr to check for sparse items, see spec-forEach

All tests are passing in all browsers that I tried:

But of course unit tests might not be covering all edge cases (like the ones you spotted and that I already fixed). I improved the test cases today to cover more edge cases and reinforce the behavior.

jdalton commented 12 years ago

'i in arr' won't normalize as in other browsers the '1 in arr' will be true while in IE 8 and lower it will be false.

jdalton commented 12 years ago

Careful to contruct proper tests as testing libs may not cover these edge cases in their API either.

millermedeiros commented 12 years ago

[edit] this comment contained wrong info!! I was looking at the wrong spec! Updated with correct info:


behavior of i in arr:

You are right, i in arr seems to work fine in all the cases but var a = [1, undefined, 3] and I don't think it's possible to normalize the behavior (damn IE). Now I think I will need to drop support for sparse arrays as well to make it consistent... creating an array like [1, undefined, 3] isn't that uncommon, I thought the issue only existed with [1, ,3] (missing item) but that isn't the case.

Thanks once again for the feedback.