Proposal for modular refactoring strategy

[lokothodida]

trafficstars

Files in the codebase are interdependent on each other in a way that is almost wholly implicit: nearly everything lives in (and hence references) the global scope. It is therefore easy to further pollute the global scope when adding features, and to completely break functionality in seemingly unrelated areas because of unclear scope-access.

This proposal aims to make the codebase more modularised without committing to a build system, whilst also being achievable in small, incremental and minimally disruptive steps that can continuously be deployed to production.

Phase 1: explicitly import dependencies: explictly export values

Take src/help.js as an example. It depends on $ (jquery), $Window (defined in src/$Window.js) and E (defined in src/helpers.js. It exposes the method show_help. It can be refactored on its own as follows, without breaking existing functionality:

// src/help.js
;(function({
    // lib/jquery.min.js
    $,
    // src/$Window.js
    $Window,
    // src/helpers.js
    E
}, exports) {

// ...rest of the existing file, then...

exports.show_help = show_help;

})(window, window);

Here I've used object destructuring to make the dependency injection look a bit nicer. Everything is still pushed into the global scope, but it is clear at the top of the file what its dependencies are, and it is clear at the bottom of the file what its exports are. Furthermore, the IIFE shields any declared properties from being exported unless it is explicitly done. This procedure can be done for effectively all of the existing files; the work is isolated to one file, and can be done concurrently for multiple files across different PRs.

Note that even at this early stage, said modularisation makes unit-testing more plausible, because we know exactly what properties/objects need to be mocked for the window object in each file.

Phase 2: group dependencies under a namespace

Once every file has made its dependencies explicit, it is then possible to group a file's exports under a namespace. For now, it would be fine to put things under something like jspaint.file_name. So looking back at src/help.js, we can export the module to jspaint.help:

// src/help.js
window.jspaint = window.jspaint || {};
;(function({
    // lib/jquery.min.js
    $,
    // src/$Window.js
    $Window,
    // src/helpers.js
    E
}, exports) {

// ...rest of the existing file, then...

exports.show_help = show_help;

})(window, window.jspaint.help = {});

Then in the modules where show_help is used (e.g. src/menu.js), you import jspaint.help and destructure it to get show_menu:

// src/menu.js
;(function({
    // dependencies that haven't been namespaced yet
}, {
    // jspaint namespace passed here as an argument, and the help module is extracted
    help
}, exports) {

// Extract the required dependency from the help module
const { show_menu } = help;

// ...rest of the file as normal, then...

exports.menus = menus;

})(window, window.jspaint, window);

This would need to be done at the level of a whole module at a time, so it would be best to do this for the smaller modules first (i.e. the ones which have the fewest exports and are used in the fewest files). This would iteratively be done until eventually no module exports directly onto window, and the only explicit imports directly from window are for libraries (e.g. jquery).

Phase 3: using an established module format

Once the modules have all been isolated appropriately by the above steps, translating them into a particular module format (i.e. ES6, CommonJS, AMD) is simple because it is just a case of translating the syntax (i.e. turning the dependency injection into the respective require/import structure, and the exports into the corresponding exporting structure). This would allow the project to be loaded through one root file, and let the module loader handle the ordering of the dependencies, rather than manually ordering the <script> tags. This doesn't mean that the project will need a build step: using SystemJS we can use any of the popular module formats and load them in the browser asynchronously. Because the order of the loading no longer matters, this wouldn't cause much of an issue (except perhaps with the initial loading of the theme, but again there are workarounds for what to show the user on the initial draw of the page).

---

This is just a proposal draft, and I'm making it because this project is awesome and I definitely want to make more contributions to it. But I also want it to be easier in the future for others to make contributions, and it is worth opening a dialog about how to break the codebase down into smaller, isolated, more manageable chunks without doing for a massive revamp or re-write.

[lokothodida]

Some notes on possible pain points with this strategy:

Statefull variables

At first, some modules won't be easily refactored because they depend on the current state of the app (e.g. the variable selected_tool is used in multiple files outside of src/app.js). Variables like this may need to be left as global until something else can be done about them (e.g. finding a way for the state to be passed in as an argument from within src/app.js? Otherwise they may need to just stay global, albeit explicitly located in some kind of state object).

Order of loading

Some modules depend on the existence of variables after the module has been loaded (e.g. $Component depends on $left, which is defined later on in src/app.js. Again, variables like this may need to be global for some time until they are refactored separately.

[1j01]

Hey, I really appreciate your taking the time to write this.

Back in 2022, I wrapped a lot of JS files in IIFEs, in a0e119ba and subsequent commits, following your Phase 1 advice, although only tracking exports and not imports.

I've now begun converting scripts to ES Modules, using the following pattern:

// @ts-check
/* global $canvas, $Component, $left, $right, $status_text, get_direction, localize, main_canvas, return_to_tools, select_tool, select_tools, selected_tool, selected_tools */
import { $G, E, make_css_cursor } from "./helpers.js";
import { get_theme } from "./theme.js";

...

export { $ToolBox };
// Temporary globals until all dependent code is converted to ES Modules
window.$ToolBox = $ToolBox;

I had to first mark all (or most) of my scripts with defer so that the order is preserved as I start to convert scripts to type="module" since module scripts are implicitly deferred. Then, by keeping global exports around alongside ESM exports, I can progressively transition scripts to ESM.

I have ESLint configured to ignore unknown globals generally, but I'm re-enabling reporting of global access for the set of files converted to ESM so that I'm aware of any globals I'm using. (In particular, attempts to write to global variables without a window. prefix would fail, so it's important to catch.)

	"overrides": [
		{
			files: [
				"src/theme.js",
				"src/helpers.js",
				"src/$ToolBox.js",
				"src/$FontBox.js",
				"src/$ColorBox.js",
				"src/OnCanvasObject.js",
				"src/OnCanvasHelperLayer.js",
				"src/OnCanvasSelection.js",
				"src/OnCanvasTextBox.js",
				"src/Handles.js",
				"cypress/**/*.js",
			],
			parserOptions: { sourceType: "module" },
			rules: {
				"no-undef": "warn",
				"no-unused-vars": "error",
			},
		}
	]

I'm also adding JSDoc for typechecking as I go. This lets me keep the code as JS while adding type safety. I went with this approach because, while I'm familiar with TypeScript, I don't want to convert the code to TypeScript because 1. I've mostly managed to avoid adding any compile steps to the project, and 2. "JS" is in the name of the project! It's not "TS Paint", after all!

It's gonna be a bit messy until everything is ESM and then it should be able to be cleaned up pretty swiftly, I think.

At any rate, I've just pushed to master with a smattering of scripts converted to ESM.

[1j01]

Correction: Apparently ES Modules can write to globals defined externally. demo fiddle

<body>
	<script>
		window.foo = "";
		let i = 0;
		setTimeout(()=> { console.log({foo, i}); }, 500);
	</script>
	<script type="module">
		foo = "bar";
		for (i = 0; i < 10; i++) {
			console.log(i);
		}
	</script>
</body>

This actually makes things easier, as I don't need to figure out an alternative way to set/store global app state until I convert the file that owns the state to ESM.

[1j01]

The app is now 99% ES Modules, at least nominally. I've saved the hardest parts for last:

• I moved most of the stateful globals into a separate file app-state.js in order to convert the remainder of app.js to ESM.
• I delayed converting app-localization.js, since it uses document.write to inject a script to load localization data synchronously.

For app-state.js, theoretically I could convert it to ESM if I just change all the variables like brush_shape to window.brush_shape, but that wouldn't be very meaningful. But wrapping them in a state object should be more meaningful.

For app-localization.js, I have tried (on a branch) replacing the script injection with fetch and top-level await, but I ran into circular dependency issues. It's meant to show an error message if localization data fails to load, but message boxes are meant to have a localized default title. So at least in the case of localization data failing to load (and ideally only that case), it needs to skip localizing the title. My plan is, roughly, to delay importing app-localization.js until creating a dialog, and then use dynamic import() but ignore the error if it fails to import, falling back to an unlocalized title. An error message should already be shown for that failure, or likely is being shown with the message box being opened. (And theoretically, the dynamic import of should be cached and app-localization.js should only be loaded once.)

It's frankly a little hard for me to wrap my brain around this fully right now, and writing this down is helping, but there's also the complexity of interoperating with 98.js.org, which I think I've overcomplicated and can simplify but it's hard to say I'm not missing something. (Specifically, I have 98.js.org injecting window.showMessageBox into the iframe, so that API must be synced up, but also I'm using a global window.defaultMessageBoxTitle which 98.js.org looks for within the iframe, in its showMessageBox implementation. But I think I could just have the app wrap the injected showMessageBox to provide the default title instead of replacing the showMessageBox function entirely.)

Anyways, I have also split out several modules along the way, but there's a lot more to go after this, like decoupling the canvas interactivity from the rest of the app logic. It's coming along pretty well though! I've been working hard at this.