diff --git a/front/src/js/directives/offendersDirectives.js b/front/src/js/directives/offendersDirectives.js index c147520..df2b35c 100644 --- a/front/src/js/directives/offendersDirectives.js +++ b/front/src/js/directives/offendersDirectives.js @@ -109,7 +109,7 @@ } else if (context.length > 3) { html += ' and ' + (context.length - 2) + ' more...'; } - return html + '}'; + return html + ')'; } function isJQuery(node) { @@ -670,10 +670,8 @@ if (node.data.callDetails.context && node.data.callDetails.context.length === 0) { html += '

Called on 0 jQuery element

Useless function call, as the jQuery object is empty.

'; - } else if (node.data.type === 'jQuery - bind' && node.data.callDetails.context.length > 3) { - html += '

The .bind() method attaches the event listener to each jQuery element one by one. Using the .on() method with event delegation is preferable if available (from v1.7).

'; - } else if (node.data.type === 'jQuery - on' && node.data.callDetails.context.length > 3) { - html += '

The .on() method used this way attaches the event listener to each jQuery element one by one. Using the event delegation version of the method is preferable if available (from v1.7).

'; + } else if (node.eventNotDelegated) { + html += '

This binding should use Event Delegation instead of binding each element one by one.

'; } if (node.data.resultsNumber === 0) { diff --git a/front/src/views/rule.html b/front/src/views/rule.html index 5badf0a..b54833a 100644 --- a/front/src/views/rule.html +++ b/front/src/views/rule.html @@ -83,6 +83,21 @@ Function {{offender.functionName}} used {{offender.count}} times +
+ function {{offender.functionName}} used on {{offender.contextLength}} DOM elements without event delegation +
no backtrace
+
+ backtrace +
+
+ {{obj.functionName}}() + + line {{obj.line}} +
+
+
+
+
{{offender.writeFn}} diff --git a/lib/metadata/policies.js b/lib/metadata/policies.js index f7877a5..a203b1c 100644 --- a/lib/metadata/policies.js +++ b/lib/metadata/policies.js @@ -439,6 +439,15 @@ var policies = { "isAbnormalThreshold": 0, "hasOffenders": true }, + "jQueryNotDelegatedEvents": { + "tool": "jsExecutionTransformer", + "label": "Events not delegated", + "message": "

This is the number of events that are bound with the .bind() or the .on() function without using event delegation.

This means jQuery binds each element contained in the object one by one. This is bad for performance.

", + "isOkThreshold": 1, + "isBadThreshold": 100, + "isAbnormalThreshold": 180, + "hasOffenders": true + }, "cssParsingErrors": { "tool": "phantomas", "label": "CSS syntax error", diff --git a/lib/metadata/scoreProfileGeneric.json b/lib/metadata/scoreProfileGeneric.json index df7400e..aa2590c 100644 --- a/lib/metadata/scoreProfileGeneric.json +++ b/lib/metadata/scoreProfileGeneric.json @@ -38,9 +38,10 @@ "jQuery": { "label": "jQuery", "policies": { - "jQueryVersion": 10, + "jQueryVersion": 1, "jQueryVersionsLoaded": 1, - "jQueryFunctionsUsed": 1 + "jQueryFunctionsUsed": 1, + "jQueryNotDelegatedEvents": 1 } }, "cssComplexity": { diff --git a/lib/tools/jsExecutionTransformer.js b/lib/tools/jsExecutionTransformer.js index b15ccd1..6f5a07f 100644 --- a/lib/tools/jsExecutionTransformer.js +++ b/lib/tools/jsExecutionTransformer.js @@ -13,16 +13,24 @@ var jsExecutionTransformer = function() { var metrics = { DOMaccesses: 0, DOMaccessesOnScroll: 0, - queriesWithoutResults: 0, - jQueryCalls: 0, - jQueryCallsOnEmptyObject: 0, - jQueryNotDelegatedEvent: 0 + queriesWithoutResults: 0 }; var offenders = { }; + var hasjQuery = (data.toolsResults.phantomas.metrics.jQueryVersionsLoaded > 0); + if (hasjQuery) { + metrics.jQueryCalls = 0; + metrics.jQueryFunctionsUsed = 0; + metrics.jQueryCallsOnEmptyObject = 0; + metrics.jQueryNotDelegatedEvents = 0; + + offenders.jQueryFunctionsUsed = []; + offenders.jQueryNotDelegatedEvents = []; + } + try { debug('Starting JS execution transformation'); @@ -31,11 +39,17 @@ var jsExecutionTransformer = function() { if (javascriptExecutionTree.children) { javascriptExecutionTree.children.forEach(function(node) { - var contextLenght = (node.data.callDetails && node.data.callDetails.context) ? node.data.callDetails.context.length : null; + var contextLength = (node.data.callDetails && node.data.callDetails.context) ? node.data.callDetails.context.length : null; - if ((node.data.type === 'jQuery - bind' || node.data.type === 'jQuery - on') && contextLenght > 3) { - metrics.jQueryNotDelegatedEvent += contextLenght - 1; + if (isABindWithoutEventDelegation(node, contextLength)) { + metrics.jQueryNotDelegatedEvents += contextLength; + offenders.jQueryNotDelegatedEvents.push({ + functionName: node.data.type.substring(9), + contextLength: contextLength, + backtrace: offendersHelpers.backtraceToArray(node.data.backtrace) + }); node.warning = true; + node.eventNotDelegated = true; } if (node.data.resultsNumber === 0) { @@ -43,7 +57,7 @@ var jsExecutionTransformer = function() { node.warning = true; } - if (contextLenght === 0) { + if (contextLength === 0) { metrics.jQueryCallsOnEmptyObject ++; node.warning = true; } @@ -87,9 +101,7 @@ var jsExecutionTransformer = function() { }); // Count the number of different jQuery functions called - if (data.toolsResults.phantomas.metrics.jQueryVersionsLoaded) { - metrics.jQueryFunctionsUsed = 0; - offenders.jQueryFunctionsUsed = []; + if (hasjQuery) { jQueryFunctionsCollection.sort().forEach(function(fnName, cnt) { if (fnName === 'jQuery - find') { @@ -181,6 +193,30 @@ var jsExecutionTransformer = function() { return count; } + + function isPureString(str) { + return typeof str === 'string' && str[0] !== '{' && str !== '(function)' && str !== '[Object]' && str !== '[Array]' && str !== 'true' && str !== 'false' && str !== 'undefined' && str !== 'unknown' && str !== 'null'; + } + + function isABindWithoutEventDelegation(node, contextLength) { + // Count only on larger bindings + if (contextLength <= 3) { + return false; + } + + if (node.data.type === 'jQuery - on' && node.data.callDetails.arguments[1] && !isPureString(node.data.callDetails.arguments[1])) { + return true; + } + + if (node.data.type.indexOf('jQuery - ') === 0 && node.children && node.children.length === 1) { + var child = node.children[0]; + if (child.data.type === 'jQuery - on' && child.data.callDetails.arguments[1] && !isPureString(child.data.callDetails.arguments[1])) { + return true; + } + } + + return false; + } }; module.exports = new jsExecutionTransformer(); \ No newline at end of file diff --git a/test/www/jquery-page.html b/test/www/jquery-page.html index 34f53cc..f2e3f16 100644 --- a/test/www/jquery-page.html +++ b/test/www/jquery-page.html @@ -20,6 +20,7 @@ // Let's start the spaghetti code!!! + $('li').bind('mouseover', function() {}); $('ul').find('li'); $('li', $('#foo')); var foo = document.getElementById('foo');