Report: group event listener extended info by call site location #960
Conversation
| @@ -6,6 +6,7 @@ | |||
| .http-resource__code { | |||
| text-overflow: ellipsis; | |||
| overflow: hidden; | |||
| white-space: pre-line; | |||
There was a problem hiding this comment.
this fixes some of the spacing issues on the code snippets. Still not perfect though.
| const isMutationEvent = this.MUTATION_EVENTS.indexOf(loc.type) !== -1; | ||
| const sameHost = loc.url ? url.parse(loc.url).host === pageHost : true; | ||
| return sameHost && isMutationEvent; | ||
| }).map(EventHelpers.addFormattedCodeSnippet); | ||
|
|
||
| results = EventHelpers.groupCodeSnippetsByLocation(results); |
There was a problem hiding this comment.
maybe just create a new variable (groupedResults or whatever)?
| @@ -76,6 +76,8 @@ class PassiveEventsAudit extends Audit { | |||
| !mentionsPreventDefault; | |||
| }).map(EventHelpers.addFormattedCodeSnippet); | |||
|
|
|||
| results = EventHelpers.groupCodeSnippetsByLocation(results); | |||
| /** | ||
| * Groups event listeners under url/line/col "violation buckets". | ||
| * | ||
| * @param {!Array<Object>} listeners Results from the event listener gatherer. |
There was a problem hiding this comment.
Object is so vague it doesn't really matter, but technically could do {!Array<!Object>} :)
| * A copy of the original listener object with the added properties. | ||
| */ | ||
| function groupCodeSnippetsByLocation(listeners) { | ||
| // The listener gatherer returns a list of (url/line/col) src locations where |
There was a problem hiding this comment.
maybe move this comment up (slightly modified) to the function description?
| const map = new Map(); | ||
| listeners.forEach(loc => { | ||
| const key = JSON.stringify( | ||
| {line: loc.line, col: loc.col, url: loc.url, type: loc.type}); |
There was a problem hiding this comment.
this looks like it will fit on one line?
| const results = []; | ||
| for (const item of map.entries()) { | ||
| const lineColUrlObj = JSON.parse(item[0]); | ||
| const listenersForLocation = item[1]; |
There was a problem hiding this comment.
if you do map.forEach you get named arguments instead of having to name them with these extra lines and generic item[i]
| // part of the codebase). Instead of showing a repetitive list of | ||
| // url/line/col violations, we make each trio a unique bucket and | ||
| // display the user's event handlers under each. | ||
| const map = new Map(); |
There was a problem hiding this comment.
more descriptive variable name. locToListenersMap or something?
| * | ||
| * @param {!Array<Object>} listeners Results from the event listener gatherer. | ||
| * @return {!Array<Object>} | ||
| * A copy of the original listener object with the added properties. |
There was a problem hiding this comment.
not really a copy of the original object anymore
| }, ''); | ||
| lineColUrlObj.code = codeSnippets; | ||
| // All listeners under this bucket have the same line/col. We use the first | ||
| // one as the label for all of them. |
There was a problem hiding this comment.
maybe We use the first's label as the label for all of them.?
| * Groups event listeners under url/line/col "violation buckets". | ||
| * | ||
| * @param {!Array<Object>} listeners Results from the event listener gatherer. | ||
| * @return {!Array<Object>} |
There was a problem hiding this comment.
@return {!Array<{line: number, col: number, url: string, type: string, code: string, label: string}>}? :)
|
PTAL |
brendankenny
changed the title
R: @brendankenny
The fix for #959 with a documented explanation of it. Tried to comment the code as much as possible :)