Rewrite of CommentTracker

[arjun-menon]

(This is a not-yet-ready PR.) @benbernard I began a significant/major rewrite of your CommentTracker a few months ago, and I wanted to get your feedback on the work so far, before investing further effort into it. Some of the highlights of this rewrite are:

• Use ES6-eqsue syntax when/where appropriate.
• Use ES6 imports + dependency management with yarn/node.
• Use custom build scripts + webpack to generate the final distributable extension.
• Migrate from Parse to the Firebase Realtime Database.
  • Fallback to Parse when comment statuses not found on Firebase.
• Separate client-server architecture with content.js + background.js.
  • The background.js deals with communicating w/ Parse & Firebase.
  • The content.js deals with manipulating the DOM / all the UI stuff.
• No more polling Parse for updates.
  • Relies on Firebase websocket-based notifications for comment resolution updates.
• No more polling the DOM for changes with setInterval.
  • Use DOM MutationObservers. (The file waitForKeyElements.js has been eliminated.)
• Added a nice new icon (that I created) for this extension. 🙂
• Added ESLint rules based on the Standard Style.
• Significantly refactored to be cleaner, easier to understand, and easier to reason about.
  • I use RxJS (Reactive Extension for JS) to achieve a lot of this.
  • I've gotten rid of underscore.js by using comparable functions from RxJS.
• This might be a negative, but I removed the feature where adding a new comment to a thread makes that thread "Unresolved". (As I found this to be more annoying than useful. 😕 )

There is a lot more work I have planned, including a significant reworking/improvement of UI/UX of this extension, adding Flow type checking to the code, maybe moving away from jQuery to something lighter, etc.

☑️ Firefox Support Saga

This extension (with these changes) will now work out-of-the-box 😂 on Mozilla Firefox. 😄

However, getting it to work on Firefox was a real pain. 😅 The biggest problem was that if an extension talks to a cross-origin server (in our case, Firebase/Parse), and the web page specifies a CSP (Content Security Policy), like GitHub does, then Firefox applies the CSP to the extension's content scripts as well, whereas Chrome does not. This is a bug that they've acknowledged, but have not fixed in over a year: https://bugzilla.mozilla.org/show_bug.cgi?id=1267027

This bug made it very painful to migrate this extension to Firefox, as the only workaround was to create both a background script and a content script, with the talking to the server happening in the background script, and then having the background script act as a pipe, and relay information back and forth to/from the content script. (It's ridiculous.) I had a fair bit of trouble getting the communication between content page(s) and the background page working.

But a small positive to this added complexity, is that the extension is more efficient now. The instance of Firebase/Parse library is no longer duplicated in every tab. All of the GitHub tabs now share the same instance of Firebase/Parse. So this likely leads to lowered network socket usage (assuming one instance uses a single WebSocket for all communication), and likely to lowered memory (and CPU) usage as well.

The other problem I had when migrating to Firefox, had to do with using libraries. In Chrome, I could just list the library before the script, and it would just work. For example, I could just do: content_scripts: ['Rx.min.js', 'content_script.js'], and it worked. In this case, with Rx.min.js, I was able to refer to Rx without any trouble inside my content script in Chrome. However, in Firefox, it didn't work –– Rx was simply an undefined variable. Even though Rx.min.js was something that you could include in a page with a <script> tag, so it should have set the Rx variable. Oh well... so I ended up using webpack instead and 'a more proper' dependency management system with yarn/node to be able to use Rx inside the content script on Firefox.

[benbernard]

Wow! This is very cool.

Love most things here, though I'll probably want to tweak the eslint rules. But I'm okay with everything else.

I just merged several pull requests that add some functionality from @alexbt which did a bunch of things that you will want to pull in

• Makes resolved/unresolved markers appear on the "Files" tab
• Makes a list of unresolved comments appear at the bottom of the PR pages (botht he commits tab and the files tab)
• Makes a better options page (though the option on it is obsolete on your version b/c of no polling)

So it would be great not to lose that work.

Also, my team basically made this to track when new comments come in on old threads, so I can't merge if you feel we need to keep things resolved when a new comment comes in... Perhaps could be an option to turn that off? (With it defaulting to on)

This is so much work! Thank you so much for doing all of this! Its incredible. I know well the pains of getting Firefox extensions to work too, and it sounds like you put a lot of effort in. Just a few more tweaks and we can run with it.

Maybe one day I can write tests for this... sigh

[arjun-menon]

I've created a commit (f34b03a) that temporarily reverted the contributions @alexbt made. I'll work on adding them (i.e. whichever are still applicable) back on top of my changes. 🏃

I'll also work on restoring the feature where if new a comment is added to a thread, the thread becomes "Unresolved". Perhaps, instead of having an options change, it could be controlled through a context menu displayed when the user clicks on the extension button/icon. 👍

[benbernard]

Hey arjun, just checking on the status of this, if you still want to get this in, happy to help / give guidance :). Totally understand if real work/life has side tracked this for you (I'm about to do some work on this to fix it for recent github changes)