Skip to content

Commit

Permalink
reverted the isFunction() and isDefined() refactor PR since you can't…
Browse files Browse the repository at this point in the history 
… pass undefined/undeclared vars to functions in JS
  • Loading branch information
@protodave
protodave committed Jun 27, 2014
1 parent fd321d6 commit f6eb3a7
Showing 1 changed file with 6 additions and 14 deletions.
20 changes: 6 additions & 14 deletions svg-injector.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,6 @@
}
};

function isFunction(value) {
return typeof value === 'function';
}

function isDefined(value) {
return typeof value !== 'undefined';
}

// SVG Cache
var svgCache = {};

Expand Down Expand Up @@ -93,7 +85,7 @@
};

var loadSvg = function (url, callback) {
if (isDefined(svgCache[url])) {
if (svgCache[url] !== undefined) {
if (svgCache[url] instanceof SVGSVGElement) {
// We already have it in cache, so use it
callback(cloneSvg(svgCache[url]));
Expand Down Expand Up @@ -229,7 +221,7 @@
// Load it up
loadSvg(imgUrl, function (svg) {

if (!isDefined(svg) || typeof svg === 'string') {
if (typeof svg === 'undefined' || typeof svg === 'string') {
callback(svg);
return false;
}
Expand Down Expand Up @@ -374,19 +366,19 @@
var eachCallback = options.each;

// Do the injection...
if (isDefined(elements.length)) {
if (elements.length !== undefined) {
var elementsLoaded = 0;
forEach.call(elements, function (element) {
injectElement(element, evalScripts, pngFallback, function (svg) {
if (isFunction(eachCallback)) eachCallback(svg);
if (eachCallback && typeof eachCallback === 'function') eachCallback(svg);
if (done && elements.length === ++elementsLoaded) done(elementsLoaded);
});
});
}
else {
if (elements) {
injectElement(elements, evalScripts, pngFallback, function (svg) {
if (isFunction(eachCallback)) eachCallback(svg);
if (eachCallback && typeof eachCallback === 'function') eachCallback(svg);
if (done) done(1);
});
}
Expand All @@ -402,7 +394,7 @@
module.exports = exports = SVGInjector;
}
// AMD support
else if (isFunction(define) && define.amd) {
else if (typeof define === 'function' && define.amd) {
define(function () {
return SVGInjector;
});
Expand Down

3 comments on commit f6eb3a7

@stryju
Copy link
Contributor

@stryju stryju commented on f6eb3a7 Jun 27, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

erm... i think you can :-)

isFunction() and isDefined() would both return false - which is the purpose for that

or am i missing something?

@protodave
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you pass an undeclared variable to a function in JavaScript it throws a ReferenceError.

function check(varToCheck) {}
check(aVariableThatIsNotDeclared); // ReferenceError: aVariableThatIsNotDeclared is not defined

So your PR was throwing an error here:

// AMD support
else if (isFunction(define) && define.amd) { // ReferenceError: define is not defined

Since define wasn't... defined ;}
(for me at least, since I wasn't using require.js)

typeof is an operator instead of a function, so we can use that to test if a var exists.

For the rest of the calls... you are right, they would be fine since those vars are actually declared (though might not be defined). I think what I might end up doing is just making sure vars exist with some defaults that work without needing so many checks, like:

elements = elements || []
eachCallback = eachCallback || new Function()

@stryju
Copy link
Contributor

@stryju stryju commented on f6eb3a7 Jun 28, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, right!

in this case i would go

if (define && isDefined(define)) { ... }

Please sign in to comment.