3

Am trying to implement w2ui multi select in one of d3 charts which am working on.

This is the link to sample jsfiddle with the problem.

I have three function :

//get a column of an array
Array.prototype.getColumn = function(name) {
  return this.map(function(el) {
    // gets corresponding 'column'
    if (el.hasOwnProperty(name)) return el[name];
    // removes undefined values
  }).filter(function(el) {
    return typeof el != 'undefined';
  });
};
//remove duplicates in an array
Array.prototype.contains = function(v) {
  for (var i = 0; i < this.length; i++) {
    if (this[i] === v) return true;
  }
  return false;
};
Array.prototype.unique = function() {
  var arr = [];
  for (var i = 0; i < this.length; i++) {
    if (!arr.contains(this[i])) {
      arr.push(this[i]);
    }
  }
  return arr;
}

I need to implement these three in one of my functions.

Problem is that, whenever I try to implement these functions with Array.prototype I get items in the multiselect as "undefined". The number of "undefined" is directly propotional to the number of functons with Array.prototype functions.

If I remove these functions, I can get the multi-select to work properly(only the multi-select part, not the chart as a whole. I don't understand, what's causing the error.

Any help is appreciated. Thanks.

altocumulus
  • 21,179
  • 13
  • 61
  • 84
driftking9987
  • 1,673
  • 1
  • 32
  • 63
  • 6
    And you've just learned why it's a bad idea to modify native prototypes. Now make those regular functions instead. – adeneo Jun 19 '16 at 22:12
  • 1
    And you've just learned why it's a bad idea to do anything without having a whole plan for your execution environment. You can modify native prototypes with no problem as long as you require coding standards that don't conflict with that decision. –  Jun 19 '16 at 22:17
  • So If I implement the functionalities as separate function, it'll work? – driftking9987 Jun 19 '16 at 22:36
  • Libraries that don't respect the fact that JS is a shared environment may, for example, use `for-in` to iterate an Array. Because this includes all enumerable properties *(including inherited ones)*, it can bump into your extensions. Personally, I'd file a [bug report](https://github.com/vitmalina/w2ui/issues), but that's up to you. Or just take adeneo's advice. –  Jun 19 '16 at 22:43
  • ...and by the way, this problem isn't unique to custom extensions. You'd have the same problem with polyfills, and they should take that into account if their library supports IE8. –  Jun 19 '16 at 23:06
  • w2ui contributor here. This should be fixed in w2ui 1.5, see my pull request from March: https://github.com/vitmalina/w2ui/pull/1194/files - you're linking an old 1.5 JS in your jsfiddle, if you use the one from the `dist` folder it works: http://jsfiddle.net/4z4dhpyh/1/ – Mike Scotty Jun 20 '16 at 07:21
  • @mpf82, the jsfiddle which you posted, is not working. The drop down is not coming in that. And should I take both `w2ui.css` and `w2ui.js` from the dist folder? – driftking9987 Jun 20 '16 at 08:49
  • @squint, thanks a lot for the info. i'll try to come up with my own implementation as told by adeneo in sometime. Thanks. – driftking9987 Jun 20 '16 at 08:51
  • Chrome was complaining about the mime type, worked fine in FF. Updated fiddle: http://jsfiddle.net/4z4dhpyh/2/ - and yes, you should use the latest js and css from the dist folder. But note that 1.5 is not (yet) released as stable. The latest stable is 1.4.3 which you can get from the w2ui homepage. – Mike Scotty Jun 20 '16 at 09:04
  • @driftking9987: You're welcome. Since this library seems to not support IE8, you should be safe using the answer provided below. This makes the methods you add non-enumerable, so there won't be any collision. Good to see they're fixing it though. Looking at the source, they seemed pretty responsible about iterating arrays. –  Jun 20 '16 at 10:22
  • ...also, just FYI, ES7 has an `.includes()` method that is basically the same as your `.contains()`. A [patch is here](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/includes#Polyfill), but again, use `Object.defineProperty` as below. –  Jun 20 '16 at 10:28
  • Considering all the fact, I decided to implement my own functions rather than using `Array.prototype`. As mentioned by @mpf82, the w2ui is not stable one so I guess there is not point taking risk. And the given answer is correct but then it's a workaround, I guess i'll be better away with my own implementation. – driftking9987 Jun 20 '16 at 11:59
  • Taking your functions off the `.prototype` is definitely a valid solution, though I wouldn't call the answer below a workaround. It's actually the way it should be done in the first place. –  Jun 20 '16 at 17:13

1 Answers1

9

In general messing with the core javascript objects is a bad idea when you are working with the third party libraries. If you still want to keep it this way and solve this particular problem use the Object.defineProperty method, turning off the enumerable bit

So for example change 

Array.prototype.contains = function(v) {
  for (var i = 0; i < this.length; i++) {
    if (this[i] === v) return true;
  }
  return false;
};

to 

Object.defineProperty(Array.prototype, 'contains', {
    enumerable: false,
    value: function(v) {
        for (var i = 0; i < this.length; i++) {
            if (this[i] === v) return true;
        }
        return false;
    }
});

and similar for other prototype method that you have added.

Hector Barbossa
  • 5,506
  • 13
  • 48
  • 70
  • Thanks @Hector, it does solves the issue, but I implemented another function to get the things done rather than using `Array.prototype`. Since it's working solution, i'll accept the answer. Might help someone. – driftking9987 Jun 20 '16 at 12:01
  • addition: you may want to add the property `writable: true,` (e.g. below `enumerable: false`) to ensure compatibility with other libraries that redefine prototype methods, otherwise they'll cause an error like `Uncaught TypeError: Cannot assign to read only property 'contains' of object...`. See https://github.com/opensheetmusicdisplay/opensheetmusicdisplay/issues/980 – sschmidTU Dec 22 '22 at 15:30