MrOtherGuy / fx-autoconfig

Load custom javascript in browser context
Mozilla Public License 2.0
164 stars 10 forks source link

@onlyonce doesn't work without @startup anymore, but the readme isn't clear on that #15

Closed aminomancer closed 2 years ago

aminomancer commented 2 years ago

This commit changed loadScript to tryLoadIntoWindow and how the onlyonce directive is handled. So probably the readme should say that it now requires the startup directive too.

Actually, I still feel like it'd be more useful to have a directive that just runs the entire script only once. I have a couple scripts that register JSActors, so their entire behavior is only supposed to execute once, but they won't work before startup tasks. I wouldn't be inclined to wrap it all in a startup function so I will probably just check whether the actors are registered before registering them.

aminomancer commented 2 years ago

I misunderstood. What actually needs to happen for it to work is just that a valid startup directive exist. So I guess the intention here is that we only run the script once, but run the startup function repeatedly. But in that case, it's fine to just have an empty startup function, so it's simpler to just not pass a startup directive at all. I'll make a suggestion:

  tryLoadIntoWindow(win) {
    if (this.inbackground || !this.regex.test(win.location.href)) {
      return
    }
    try {
      if(this.onlyonce && this.isRunning) {
        if (this.startup) {
          SHARED_GLOBAL[this.startup]._startup(win)
        }
        return
      }

      Services.scriptloader.loadSubScriptWithOptions(
        `chrome://userscripts/content/${this.filename}`,
        {
          target: win,
          ignoreCache: this.ignoreCache
        }
      );

      this.isRunning = true;
      this.startup && SHARED_GLOBAL[this.startup]._startup(win)

    } catch (ex) {
      console.error(new Error(`@ ${this.filename}`,{cause:ex}));
    }
  }
MrOtherGuy commented 2 years ago

So I guess the intention here is that we only run the script once, but run the startup function repeatedly.

Yes. That's how it's supposed to work, although I admit the documentation nor the directive name "@startup" aren't very clear about this intention.

But yeah, this is totally a bug because right now an onlyonce script without startup will still be executed fully for each window.

Thanks for pointing this out!