feat: Run features in alphabetical order; make control button order consistent

[marcustyphoon]

Description

Background: #2357.

This refactors the main content script slightly so that feature modules are resolved (simultaneously) before they're run (sequentially in alphabetical order), meaning that any code in a feature's main function is consistently run before/after code in a different feature's main function. This causes the control buttons inserted by Quick Tags and Trim Reblogs to always be in the same positions instead of randomly flipping each time a page is opened, if both features are enabled (as the mutations util is, generally speaking, order-preserving).

Naturally, the moment you start awaiting nontrivial things, stably ordering feature code gets harder, but we may as well make it stable where possible.

As noted in #2357 (reply in thread), this calls import() on the same path twice, which uses the module cache; https://github.com/AprilSylph/XKit-Rewritten/blob/master/src/main_world/index.js declines to do this by implementing an explicit cache. We could do so here, e.g.

- const importFeature = name => import(browser.runtime.getURL(`/features/${name}/index.js`));
- const importUtil = name => import(browser.runtime.getURL(`/utils/${name}.js`));
+ const moduleCache = {};
+ const importFeature = name => (moduleCache[name] ??= import(browser.runtime.getURL(`/features/${name}/index.js`)));
+ const importUtil = name => (moduleCache[name] ??= import(browser.runtime.getURL(`/utils/${name}.js`)));

(Or, you know, something that doesn't use an assignment's return value to save loc.)

There is a subtle difference (https://gist.github.com/marcustyphoon/55b336b451a64c9edb8089b296dcce24) the way the main world script uses it now, but it's inconsequential in practice and goes away if you put the ??= in an awaited function. I guess it should probably be consistent?

Screenshots

n/a

Testing steps

• Enable Quick Tags and Trim Reblogs. Refresh a page with an editable post a few times and confirm that the Quick Tags button always comes before the Trim Reblogs button.
• Confirm that enabling and disabling features works.
• Confirm that a feature like CleanFeed with automatic onStorageChanged behavior renders preference changes in real time.

[AprilSylph]

I'm approving my own code here, so please review my commits before merging. I've tested in both Firefox and Chrome.

Is it just me, or is boot actually noticeably smoother this way?

[marcustyphoon]

and actually the above code doesn't work either, since it drops the name variable out of scope. Fun!

I have terrible news.

[AprilSylph]

and actually the above code doesn't work either, since it drops the name variable out of scope. Fun!

I have terrible news.

AAAARGGHHHH!!!

[marcustyphoon]

No joke, I was just thinking about how I ought to see if there's a way to make vs code/eslint not treat name as a variable with special meaning.

Erroneous

[AprilSylph]

This is what I get for testing with the generated ZIPs—sometimes I forget I haven't loaded from my clone, make changes, reload the addon, and things still work (even though I've actually broken it).

[AprilSylph]

No joke, I was just thinking about how I ought to see if there's a way to make vs code/eslint not treat name as a variable with special meaning.

Done. Why do we even have that global!?

[marcustyphoon]

There is a subtle difference (gist.github.com/marcustyphoon/55b336b451a64c9edb8089b296dcce24) the way the main world script uses it now, but it's inconsequential in practice

Is it just me, or is boot actually noticeably smoother this way?

...Hm. You know what's interesting? The point I was making in that gist is that there's no functional difference between code that runs in the same microtask (e.g. reading from a Record/Map cache, no await gets run), running code after an await of an async function that returns an already-resolved value, and running code after a dynamic import of a module that is already loaded.

But if the code that we're running mutates the DOM in a way that would synchronously trigger a repaint, as in the case of a feature with a styleElement export here, then I actually don't know if the browser would permit multiple repaints in between callbacks that were delayed in the latter two ways. Based on my experience I would guess that it doesn't, and so it still doesn't matter, but I wouldn't say I'm 100% confident.

(To be 100% clear, the code you had committed at the time of your comment runs all styleElement insertions during the same microtask, which is totally immune to this effect, hence why I was thinking about it; it also, though, is broken and inserts no static feature stylesheets, so that would also contribute to, uh, smoothness. Then again, you probably weren't testing that code at all when you wrote that! To actually say something about smoothness, it does make some sense, assuming dynamic import of a module that is already loaded works the way I'd guess that it does.)

[marcustyphoon]

No joke, I was just thinking about how I ought to see if there's a way to make vs code/eslint not treat name as a variable with special meaning.

Done. Why do we even have that global!?

You know, jokes aside, I always wondered about that. microsoft/TypeScript-DOM-lib-generator#883 points to microsoft/TypeScript#18433 and microsoft/TypeScript#14306; I guess it's still not a totally resolved matter.

[marcustyphoon]

This is what I get for testing with the generated ZIPs—sometimes I forget I haven't loaded from my clone, make changes, reload the addon, and things still work (even though I've actually broken it).

Meanwhile, I primarily use about:debugging#/runtime/this-firefox... which means that, for Reasons™ I guess, main.js changes don't get auto-applied even though changes to almost everything else do, and it's pretty easy to forget that. So, no help there either, honestly :D

[AprilSylph]

Take two of my idea is a LoC net gain(+17 -12), so it doesn't count as a refactor: diff.patch

I was testing my actual code on Chrome, but I was doing more extensive testing on Firefox, where I had the ZIP version loaded via about:debugging.

[marcustyphoon]

Interesting; is there an advantage to that over loading the directory?

[AprilSylph]

I'm fairly certain the smoothness I was observing was indeed just a side-effect of stylesheets not being inserted. The build from the latest CI check is just as smooth about half the time, anyway.

[AprilSylph]

Interesting; is there an advantage to that over loading the directory?

This is going to sound very silly with all the above context, but it increases my confidence when issuing approvals. I can't pull the wrong PR if the way I get the PR code in the first place is from the GitHub web UI (and it lands in an empty Downloads folder). Pulling the branch directly means I have to find the correct PR in the VS Code sidebar, which is... more possible to get wrong than clicking the PR's "Checks" tab.

[marcustyphoon]

Take two of my idea is a LoC net gain(+17 -12), so it doesn't count as a refactor: diff.patch

Hm. Seems to spread out the awkwardness a bit more than, say... uh...

  const runFeatures = async function (names) {
    for (const [name, module] of await Promise.all(names.map(async name => [name, await importFeature(name)]))) {

Okay, that's still pretty ridiculous looking: three async/await keywords. Too bad https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/allKeyed is just a twinkle in tc39's eye or whatever.

I guess there's:

  const runFeatures = async function (names) {
    const modules = await Promise.all(names.map(importFeature));
    names.forEach((name, i) => {
      const {
        main,
        clean,
        stylesheet,
        styleElement,
        onStorageChanged,
      } = modules[i];
      /* */
    })
  }

Which, like, eh. And then there's... nope, we can't use Map.prototype.getOrInsertComputed(); that's thus basically what I mentioned in the PR body but worse. Hm.

[AprilSylph]

...huh, I guess there are no usages of runFeature/destroyFeature that aren't trying to run/destroy multiple features from an arbitrary array. So, on that front, making them expect an array argument makes a ton of sense.