toolkit-for-ynab / toolkit-for-ynab

A general purpose YNAB enhancing browser extension for Chrome and Firefox. Have it your way!
MIT License
1.42k stars 341 forks source link

Moving each script into the global ynabToolKit object #55

Closed Niictar closed 8 years ago

Niictar commented 8 years ago

I am working towards migrating each script that currently depends on DOM polling to use the mutationObserver object, so the scripts only fire when there is a relevant change to the page.

In order to do this, I need each feature to be defined within the window.ynabToolKit object so they can see the actOnChange() function.

So, over the next short period, I will be refactoring each script so that it looks like this:

(function poll() {
  if ( typeof ynabToolKit !== "undefined"  && ynabToolKit.pageReady === true ) {

      ynabToolKit.featureOptions.awesomeFeature = true;
      ynabToolKit.awesomeFeature = function ()  {
          // do mind blowing stuff
      };
      setTimeout(ynabToolKit.awesomeFeature, 250);

  } else {
    setTimeout(poll, 250);  
  }
})();

Or like this (when taking advantage of the actOnChange function).

(function poll() {
    if ( typeof ynabToolKit !== "undefined" && ynabToolKit.actOnChangeInit === true) {

        ynabToolKit.featureOptions.awesomeFeature = true;
        ynabToolKit.awesomeFeature = function ()  {
            // do mindblowing stuff
        };
        ynabToolKit.awesomeFeature();

    } else {
        setTimeout(poll, 250);
    }       
})();

Just an FYI so you know what is going on if I make a pull request on your feature with some refactoring. I am going to work on example 1 first and then work on migrating the relevant features to example 2 afterwards.

If you create a new feature and use example 1 right away, I wouldn't complain. Of course, this is subject to approval by the team. I have created a Trello card here: https://trello.com/c/fGZcUWiZ/9-refactor-project-moving-each-script-into-the-global-ynabtoolkit-object

iamnafets commented 8 years ago

@Niictar I attempted to do something similar for my pacing feature, but noticed that the observer would fire when the feature itself manipulated the DOM, triggering an endless loop. Is there a way to disable the observer while the feature is doing DOM updates?

thekevinbrown commented 8 years ago

Just a heads up, discussion is also happening here on this: https://trello.com/c/mEPvAQUE/3-find-a-better-way-to-observe-dom-changes-and-inject-content-either-by-hooking-ember-internals-or-by-building-a-concise-non-polli

Niictar commented 8 years ago

@iamnafets The endless loop is a pain and I've come across it myself. I got around it by getting the feature script itself to check if the work it wants to do to the DOM is already done and if so, breaks or returns out of the function call.

As @blargity is discussing in the other Trello card above, he is already looking at revamping how the features call the mutationObserver, so once that is done, we may have an easier way to avoid the endless loops, too.

thekevinbrown commented 8 years ago

This is now complete. The final task is to document what we've done, which I'll do shortly.