crccheck / raphael-svg-import-classic

Import SVG files to Raphael
MIT License
98 stars 40 forks source link

remove jQuery $.trim dependency #24

Closed konsumer closed 9 years ago

konsumer commented 9 years ago

Using String.prototype.trim to remove dependency of jQuery. This can be polyfilled for <IE9 with this:

if (!String.prototype.trim) {
  (function() {
    // Make sure we trim BOM and NBSP
    var rtrim = /^[\s\uFEFF\xA0]+|[\s\uFEFF\xA0]+$/g;
    String.prototype.trim = function() {
      return this.replace(rtrim, '');
    };
  })();
}
crccheck commented 9 years ago

I like the idea of removing jquery since trim is the only thing left, but I don't agree with using a polyfill. What about an internal trim function?

konsumer commented 9 years ago

the thing I don't like about internal versus polyfill is that the same basic standard functions get re-implemented a million times when it's really just 1 class of old browser that has the issue. Here it is, as not-a-polyfill, though:

function trim(string){
    return string.replace(/^[\s\uFEFF\xA0]+|[\s\uFEFF\xA0]+$/g, '');
}
crccheck commented 9 years ago

I used to just follow MDN docs and use polyfills, but I found that a lot people don't wrap their for ... in ... loops and then all of a sudden, their code breaks. It doesn't seem likely to me that anyone would do a for in for a string, but I'd prefer not to take that chance.

ref: https://jslinterrors.com/the-body-of-a-for-in-should-be-wrapped-in-an-if-statement

konsumer commented 9 years ago

totally fair safeguard. should I make a PR using the above internal-trim function, or submit it as a separate issue?

crccheck commented 9 years ago

I'm happy either way. I'll be able to do more in about 8 hours. The jshint config for jquery as a global should be deleted too: https://github.com/konsumer/raphael-svg-import-classic/blob/patch-2/raphael-svg-import.js#L8 /* global Raphael, $ */

konsumer commented 9 years ago

I just lumped the removal of the jshint comment in here, too.