GitHub link oneboxer for chat

The code in general is a little difficult to read. I really had an hard time. But I will be randomly jumping around on your code and reviewing it. More will be added with time.

MutationObserver:

This one is the hell on Earth to read!

Look at the nesting:

var observer = new MutationObserver(function(mutations) { //MutationObserver
 mutations.forEach(function(mutation) {
 var i;
 for (i = 0; i < mutation.addedNodes.length; i++) {
 var $addedNode = $(mutation.addedNodes[i]);
 if ($addedNode.hasClass('message')) { //if the new node is a message
 if ($addedNode.find('a').length) { //if there is a link in the message
 if($addedNode.text().trim().indexOf(' ') == -1) { //if there is no space (ie. nothing other than the link)
 if ($addedNode.find('a:last').attr('href').indexOf('github') > -1) { //if the link is to github
 var link = $addedNode.find('a:last').attr('href'); //get the link
 extractFromUrlAndGetInfo(link, $addedNode); //pass URL and added node to the function which will go on to call useApi and add the onebox
 }
 }
 }
 }
 }
 });
});

I must shake your hand: it took me 10 minutes to understand what the heck was going on!

A few points I will be making about it:

• You should always declare a variable to store the length.

  In this particular case, it may make quite a big difference.

  Compare this:

  var i;
  for (i = 0; i < mutation.addedNodes.length; i++) {
  

  To this:

  var length = mutation.addedNodes.length;
  for (var i = 0; i < length; i++) {
  

  This will speed up your code, since it doesn't have to go to a property to go to an array to get the length. The deeper the value is, the slower it will be. Since you only do this slow operation once, you will see good improvements.

• Repeated selectors.

  Selectors are a little slow. Specially this:

  $addedNode.find('a:last')
  

  That :last is a kick on your performance. I will describe what happens everytime you run it:

  • jQuery passes the selector to Sizzle
  • Sizzle tries to detect if document.querySelectorAll exists
  • It then runs document.querySelectorAll('a:last') inside a try ... catch
  • A SyntaxError is thrown
  • The syntax error is caught
  • Sizzle runs a regex to extract a
  • Fetches all the <a> elements inside that element
  • Sizzle runs the same regex to extract :last
  • Sizzle checks if it is a pseudo-selector
  • Executes the magic behind it
  • Extracts the element
  • Passes the element to jQuery

  Quite a lot of stuff going on, right? And this is repeated twice per element. You should do like this:

  $lastLink = $addedNode.find('a').last();
  

  You have no idea how fast that is compared to that whole bloat.

  To describe what happens, here it is:

  • jQuery passes the selector to Sizzle
  • Sizzle tries to detect if document.querySelectorAll exists
  • It then runs document.querySelectorAll('a') inside a try ... catch
    _{This selector will be optimized by the CSS engine, and will be way faster than document.getElementsByTagName('a'), which might happen with the selector a:last}
  • Passes the elements to jQuery
  • jQuery extracts the last element and gives it to you

  See how much processing we have cut down?

• Use early returns

  You currently do this:

  $addedNode.hasClass('message')
  

  This is the first condition. You can bail out if it isn't a .message.

• You have so much nesting going on!

  It looks like a Nest-Fest!

  You have 7 nestes stuffs! Including loops, functions and conditional blocks. That's a lot of nesting. We must reduce it!

• No need to check the length of matched elements. jQuery does nothing if there are

This is what I came up with your function:

var observer = new MutationObserver(function(mutations) { //MutationObserver
 mutations.forEach(function(mutation) {
 for (var i = 0, length = mutation.addedNodes.length; i < length; i++) {
 var $addedNode = $(mutation.addedNodes[i]);
 //If it isn't a message, bail out
 if ( !$addedNode.hasClass('message') ) {
 return;
 }
 var $lastLink = $addedNode.find('a').last();
 //If there aren't links, bail out
 if( !$lastLink.length ) {
 return;
 }
 // if there's nothing else but a link to github
 if( $addedNode.text().trim().indexOf(' ') == -1 
 && $lastLink.attr('href').indexOf('github') > -1 ) {
 //pass URL and added node to the function which will go on to call useApi and add the onebox
 extractFromUrlAndGetInfo($lastLink.attr('href'), $addedNode);
 }
 }
 });
});

I am not particularly proud about that if, but it is the best I can do.

BUT WAIT, THERE'S MORE!!!

You though I was done beating this poor function? Well, I'm not! As my last point:

• TOO MUCH JQUERY!

Yeah, you don't need jQuery for this! Watch this:

var observer = new MutationObserver(function(mutations) { //MutationObserver
 var undefined; //Required to avoid exterior changes
 //Required for Firefox compatibility
 var textProp = addedNode.innerText == undefined ? 'textContent' : 'innerText' ;
 mutations.forEach(function(mutation) {
 for (var i = 0, length = mutation.addedNodes.length; i < length; i++) {
 var addedNode = mutation.addedNodes[i];
 //If it isn't a message, bail out
 if ( !(' ' + addedNode.className + ' ').indexOf(' message ') ) {
 return;
 }
 var links = addedNode.querySelectorAll('a');
 //If there aren't links, bail out
 if( !links.length ) {
 return;
 }
 var last = links[links.length - 1]; //Last link
 // If there's nothing else but a link to github
 // More info on http://stackoverflow.com/a/9488834
 if( addedNode[textProp].trim().indexOf(' ') == -1 
 && (last.hostname == 'github.com' || last.hostname == 'www.github.com') ) {
 //pass URL and added node to the function which will go on to call useApi and add the onebox
 extractFromUrlAndGetInfo(last.href, $(addedNode));
 }
 }
 });
});

_{As an alternative to that check for the hostname, you can use last.hostname.match(/^(?:www\.)github\.com$/i) instead.}

Oh my! The performance will be MASSIVELLY IMPROVED on that handler. Event handlers should do what they have to do quickly and GTFO as soon as possible. Yours is an old lady doing 40KM/H on a highway, which is illegal.

On another answer by @rolfl, he suggests to wrap the comments in a /** @preserve */ block, like this:

/** @preserve
// ==UserScript==
.....
// ==/UserScript==
*/

This will prevent Firefox from deleting your comments, and still allows to parse them as part of the userscript.

setTimeout: (at the very end of your code)

I must say that the stuff here is a lot more readable! Thank you!

But there's still some things:

• Please, think about performance!!! Please!!!

  Not everybody has an Intel Xeon 14-core 2.6GHz CPU! Sometimes you have a little phone with a single core. You have to love performance! Marry performance!

  You have this:

  $('.message').each(function() { //loop through EXISTING messages to find oneboxable messages
  

  On my Memer project (at the time it was just a chat "translator") I do this:

  $('#chat .message:not([data-checked="1"])').foreach( [...] );
  

  Now you say: "But you said not to use pseudo-selectors!". To which I say: Correct, but this one will still be optimized by the browser because it runs on document.querySelectorAll. At least in the browsers we care about, like Chrome and Firefox. You can remove the #chat bit there if your tests show increase in speed.

  This avoids you processing the same messages over and over again.

  In fact, what I have on memer is this:

  $('#chat .message:not([data-checked="1"])')
   .attr('data-checked', 1)
   .find('>.content')
   .filter(':not(:has(.onebox))')
  

  Which goes even further to check if it isn't a oneboxed thing already. Yes, it is slower than your code, but in this case we will cheat on performance in exchange of correctness and readability. Slow and steady on this one!

• Don't mix APIs!

  A few lines below, almost at the end, you have this:

  observer.observe($('#chat')[0], { //observe with the mutation observer for NEW messages
   childList: true,
   subtree: true
  });
  

  Sure, jQuery will optimize $('#chat') into document.getElementById('chat') but it still has quite an high cost. This is pure laziness. Just write this:

  //observe with the mutation observer for NEW messages
  observer.observe(document.getElementById('chat'), {
   childList: true,
   subtree: true
  });
  

  In fact, I would do even better:

  var chatElem = document.getElementById('chat');
  setTimeout(function() {
   [...]
   //observe with the mutation observer for NEW messages
   observer.observe(chatElem, {
   childList: true,
   subtree: true
   });
  });
  

  The #chat won't go away! You can store it there and you are done! In fact, you can ever speed a bit our last point:

  $('.message:not([data-checked="1"])', chatElem)
   .attr('data-checked', 1)
   .find('>.content')
   .filter(':not(:has(.onebox))')
  

  Oh yeah! We are back on performance!

• I REPEAT: YOU HAVE TOO MUCH JQUERY!!!

  I will not re-iterate over this if. I will just de-jQuerylize(?) that whole .each():

 [...].each(function() {
 var links = this.querySelectorAll('a[href*="github.com"]');
 //Bail out if there are no links, could replace `> -1` with `~`
 if (!links.length || this[textProp].trim().indexOf(' ') > -1) {
 return;
 }
 //pass URL and message to the function which will go on to call useApi and add the onebox
 extractFromUrlAndGetInfo(links[links.length - 1].href, $(this));
 });

Stacking .replaces is bad practice; use an object instead.

return url.replace(/{username}/, details.username).replace(/{repo_name}/, details.repo_name).replace(/{issue_number}/, details.issue_number).replace(/{pull_number}/, details.pull_number);

into:

var itemsToReplace = {
 /{username}/: details.username,
 /{repo_name}/: details.repo_name,
 /{issue_number}/: details.issue_number,
 /{pull_number}/: details.pull_number
 };
for (var item in itemsToReplace){
 if (typeof itemsToReplace[item] !== 'function') {
 url = url.replace(item, itemsToReplace[item]);
 }
}
return url;

The HTML building seems like it should be improved, but, I can't think of anything much...

Although, you can put the ending +s of each line at the beginning of the next line, to increase readability.

The same thing can apply to commas in objects/arrays, which improves readability, and also being able to comment out lines easier, in case you want to temporarily enable or disable a feature.

Is my code readable easily? If not, how can I improve it?

Your code is mostly readable, but, the massively nested block and the HTML building are a bit of an eyesore.

Is there a cleaner way to do something (anything!) in the script? For example, to check whether there is another word, I've used indexOf(' ') - is there a more preferable way to check for multiple words?

Hmm... Perhaps: string.split(' ').length > 1 instead?

RE the replace, here's something I came up with which has no destructive assignment or side effects. I might be able to go into more depth later on with regards to the rest of your code, but right now I'm at work.

function replaceUrl(originalUrl, details) {
 var itemsToReplace = [
 { test: /{username}/, with: details.username },
 { test: /{repo_name}/, with: details.repo_name },
 { test: /{issue_number}/, with: details.issue_number },
 { test: /{pull_number}/, with: details.pull_number },
 ];
 var url = itemsToReplace.reduce(function(previous, current) {
 return previous.replace(current.test, current.with);
 }, originalUrl);
}

You'll need to set the original url to originalUrl, of course. This doesn't use side effects and treats originalUrl as if it's immutable, which is useful if you're using ES6 because then you can declare it as const as opposed to let/var

• javascript
• stackexchange
• chat
• userscript