Convert to ES6 module syntax

[eritbh]

All our platforms support the module specification from ES6, and also support the currently stage 4 proposal for dynamic import(). These are all we need to convert the existing module structore to actual modules, allowing us to greatly simplify the init process.

Rather than waiting for an event and registering themselves, each module can export a TBModule object which will be imported and registered elsewhere. This will also allow us to avoid IIFEs wrapping everything, because module code doesn't expose declarations to other files unless they are explicitly exported.

We can't make content scripts act as modules straight away, but we can use an async function to import() module code. Reference: https://stackoverflow.com/questions/48104433/how-to-import-es6-modules-in-content-script-for-chrome-extension

[eritbh]

I think this issue should also cover a restructuring of how we define modules. A couple thoughts:

• We should separate "always enabled" modules like config, oldreddit, etc from the user-configurable modules in some way. Currently, "module" is used in a couple different senses, meaning both "a part of Toolbox with its own options that users can disable", or "a chunk of related code", or both, depending on context. I think it makes sense that if the user can't enable or disable something, it doesn't need all the same overhead that modules with configurable settings, etc. have.

• Maybe this is just me, but TB.Module has a kinda strange interface at the moment, requiring you to create an instance and then patch properties manually in order to set configuration and defaults for the module. I'd prefer to revise this API so that all the necessary configuration is kept as an object parameter inside the constructor, e.g.

  const self = TB.Module({
  	shortname: 'something',
  	enabledByDefault: true,
  	beta: false,
  	dev: false,
  	settings: {
  		// etc...
  	},
  }, async function init () {
  	// init code can go here
  });

• The current method of getting settings values on init is clunky. It would probably be easier to have TB.Module instances get passed a settings object on init for convenience, which would probably also require some changes to the storage code.

• TB.Module right now has a prototype get/set pair for shortname that autofills the shortname if one isn't passed explicitly. We should trash this and ensure that all modules have explicit shortnames.

• Are those module profiling methods used anywhere? If not, we should probably get rid of them too. I'd rather see a separate file for debug/perf functions like that if they do need to be used.

• The way the module list is stored at the moment is very weird. We have both an array of all modules, and also a map of shortnames to modules. We should probably replace both of these with a Set mapping shortnames to modules, since that prevents us from having to manually keep track of the same list in two places and (I think) provides faster access to a list of values than with an object.

• As far as actually implementing modules in ES6 should go, I imagine it would look something like this:

  function somePrivateHelper () {
  	// do stuff
  }
  export function somePublicHelper () {
  	// For some stuff, like usernotes, it would make sense to publish
  	// helper functions that other modules could use to interact with stuff
  }
  export default new TB.Module({
  	shortname: 'someShortname',
  	enabledByDefault: true,
  	settings: {
  		someSetting: {
  			type: 'number',
  			default: 0,
  		},
  	},
  }, async function init () {
  	// Do whatever we need to do
  });

  In general I'd like to try and avoid defining helper functions as properties of the module. If they need to be used by other modules, they can be explicitly exported; otherwise, they shouldn't be assigned as members of the module. The biggest advantage of ES6 modules is that we don't have to worry about global namespace pollution, since JS executed in module mode isn't in the global namespace. This lets us clean things up a lot.

[creesch]

We should separate "always enabled" modules like config, oldreddit, etc from the user-configurable modules in some way.

Agreed, those are now "modules" because that is the structure we now have. Having said that, some of the "core" modules do have configurable settings so in that sense I think it is logical they might share some of the same code.

I'd prefer to revise this API so that all the necessary configuration is kept as an object parameter inside the constructor, e.g.

Agreed, the example code you envisioned there does make a whole lot more sense in the modern js landscape compared to what we have now. I am not sure it ever really made sense as it was thought up by people more familiar with other languages at the time ;)

TB.Module right now has a prototype get/set pair for shortname that autofills the shortname if one isn't passed explicitly. We should trash this and ensure that all modules have explicit shortnames.

Or possibly just create shortnames automatically from the long names. Although that makes it a bit harder to rename modules if need be as settings are now shortname related.

Are those module profiling methods used anywhere?

Can be ditched as far as I am concerned.

The way the module list is stored at the moment is very weird.

Agreed here.

Implementation bit

Looks good to me.

A closing thought here is that we probably want to split things out in tasks we can already do as foundation (storage possibly, maybe module listing, etc) and work that needs to be done at the same time.

Also before we start with all this we should do a little proof of concept with the stackoverflow example you found to make sure it actually works and works as intended.

[eritbh]

export function somePublicHelper () {
	// For some stuff, like usernotes, it would make sense to publish
	// helper functions that other modules could use to interact with stuff
}

I've found a case where this would be very useful while working on #316 and #317. In order to have the modbar module open the usernotes manager for a subreddit, it needs to create a button with a specific class and data-subreddit attribute value, and the usernotes module will open the popup when clicked if that module is enabled. A much cleaner solution would be to have the usernotes module export a function for opening the manager, and let the modbar code just call that function instead.

[eritbh]

Got kinda bored and wrote a basic proof-of-concept extension demonstrating how we could structure Toolbox this way. https://github.com/Geo1088/webext-esm-poc

Unfortunately I learned this afternoon that Firefox has a bug preventing dynamic import() statements from working from within content scripts, which means that we can't use ESM in content scripts without giving up the extension context and therefore access to browser.runtime and some other stuff that we need access to. So I guess this is blocked on Firefox now. https://bugzilla.mozilla.org/show_bug.cgi?id=1536094

[k3n]

Nice PoC, you need to get bored more often 👍

[eritbh]

import() support in content scripts is apparently set to land in FF89 https://bugzilla.mozilla.org/show_bug.cgi?id=1536094#c40