feat: Rewrite module loader by oliversalzburg · Pull Request #193 · kellnerd/harmony

oliversalzburg · 2026-03-10T11:03:57Z

The previous approach suffers from provider instances losing their relation to the actual source class. This is caused by abstract base class methods that reduce instances to their base type in unexpected ways. This leads to type assumptions that ultimately don't hold up during runtime, and cause hard-to-analyze issues.

This change moves abstract methodology into the concrete types, composing merged types, and then using those composed types instead of base class references.

While this change generally seems to perform as expected and correctly (1 test is failing locally), it should likely be cleaned up to integrate better with the rest of the code structure and style. The change is primarily motivated to help illustrate the solution.

The previous approach suffers from provider instances losing their relation to the actual source class. This is caused by abstract base class methods that reduce instances to their base type in unexpected ways. This leads to type assumptions that ultimately don't hold up during runtime, and cause hard-to-analyze issues. This change moves abstract methodology into the concrete types, composing merged types, and then using those composed types instead of base class references. While this change generally seems to perform as expected and correctly (1 test is failing locally), it should likely be cleaned up to integrate better with the rest of the code structure and style. The change is primarily motivated to help illustrate the solution.

kellnerd

Thank you for opening a PR, this makes it much easier to understand what you were describing on ChatBrainz.

Unfortunately there is still an issue with your proposed typing changes. You can reproduce CI behavior locally by running deno task ok (and deno test -RE).

kellnerd · 2026-03-12T20:56:39Z

lookup.ts

 		}

-		const releaseResults = await Promise.allSettled(this.queuedReleases);
+		const releaseResults = await Promise.allSettled(this.queuedReleases.map((_) => _()));


I like how you addressed the "uncaught in promise" issue by simply wrapping the lookup function into another function which is only called when we actually want to start all the lookups.

I definitely would like to merge (or cherry-pick) this as a separate commit with some more fitting variable names, maybe something like

Suggested change

const releaseResults = await Promise.allSettled(this.queuedReleases.map((_) => _()));

const releaseResults = await Promise.allSettled(this.queuedReleaseJobs.map((job) => job()));

Agreed. Although I wasn't so sure anymore if this even provides any benefit. I would definitely move this to a separate change.

kellnerd · 2026-03-12T21:01:19Z

providers/Bandcamp/mod.ts

-	readonly releaseLookup = BandcampReleaseLookup;
+	releaseLookup(specifier: ReleaseSpecifier, options: ReleaseOptions = {}) {
+		return new BandcampReleaseLookup(this, specifier, options);
+	}


That's a nice improvement as well, I never really liked the idea of assigning a class to another class' property.

kellnerd · 2026-03-12T21:05:34Z

providers/Bandcamp/mod.ts

 import { simplifyName } from 'utils/string/simplify.js';

-export default class BandcampProvider extends MetadataProvider {
+export default class BandcampProvider extends MetadataProvider<BandcampProvider> {


Why did you (have to) use the template type parameter for some provider classes, but not for all of them?

I hope I can explain this properly, because maybe I don't understand it fully myself.

The pattern X extends Y<X> still strikes me as confusing, but is common with class inheritance in JS, because it helps to model the expected relationship into prototypes. If MetadataProvider is our abstract base class, when we interact with an instance of a derived type (like BandcampProvider), we can never properly infer that concrete type from an instance of type MetadataProvider.

That's why MetadataProvider is now MetadataProvider<Provider extends MetadataProvider<Provider> | unknown = unknown>. This potentially confusing constructs expresses that MetadataProvider without any extra information is an unknown provider with only abstract functionality available. Any class that extends MetadataProvider, can pass its concrete instance type to that base class to create a truly complete type description.

In my C++ brain, this behavior reflects passing a this pointer from a child class constructor to the base class, to allow virtual function calls, which is usually what people want to achieve when working with class inheritance.

This is important when handling multiple instances of different derived types. If all these types are reduced to MetadataProvider, or MetadataProvider<unknown>, that's a problem, because ultimately any JS object that has the same "shape" as a MetadataProvider will fit this expectation, and is reduced to the abstract functionality.

Similarly, if a function expects to receive a parameter of type BandcampProvider, that doesn't actually limit this parameter to instances of that class. You can create any const foo = {} that will have all the properties of an instance of BandcampProvider, but is not part of the prototype chain of that class (TypeScript is a lie), and then just pass that object.

In my experience, class inheritance is mostly a pain in TS/JS, and you can work much more comfortably through composition. I've written large code bases in TS with extensive class hierarchies, coming from C#, and I've had a really hard time digesting that this should be "all wrong", because I felt like it works just fine, except for a few weird/rare issues.

What really helped me was the advice to think about the type system as set theory. My thinking was around the most common denominator (the base class), and interacting with that. What you really want, is to interact with the superset of all derived classes, and then trim off the parts you don't want (through narrowing).

kellnerd · 2026-03-12T21:07:57Z

providers/Tidal/mod.ts

-	override getRelease(specifier: ReleaseSpecifier, options: ReleaseOptions = {}): Promise<HarmonyRelease> {
+	getRelease(specifier: ReleaseSpecifier, options: ReleaseOptions = {}): Promise<HarmonyRelease> {
 		if (!options.snapshotMaxTimestamp || options.snapshotMaxTimestamp > tidalV1MaxTimestamp) {
-			this.releaseLookup = TidalV2ReleaseLookup;
-		} else {
-			this.releaseLookup = TidalV1ReleaseLookup;
+			return new TidalV2ReleaseLookup(this, specifier, options).getRelease();
 		}
-
-		return super.getRelease(specifier, options);
+		return new TidalV1ReleaseLookup(this, specifier, options).getRelease();


I don't think this method is called anymore after your changes. You should port the selection logic here to the new releaseLookup method, otherwise Tidal tests should fail.

Then this is probably the reason for the failing test. I'll have another look this evening.

kellnerd · 2026-03-12T21:13:41Z

providers/registry.ts

+import type BandcampProvider from './Bandcamp/mod.ts';
+import type BeatportProvider from './Beatport/mod.ts';
+import type DeezerProvider from './Deezer/mod.ts';
+import type iTunesProvider from './iTunes/mod.ts';
+import type MoraProvider from './Mora/mod.ts';
+import type MusicBrainzProvider from './MusicBrainz/mod.ts';
+import type OtotoyProvider from './Ototoy/mod.ts';
+import type SpotifyProvider from './Spotify/mod.ts';
+import type TidalProvider from './Tidal/mod.ts';
+
+export type MetadataProviders =
+	| BandcampProvider
+	| BeatportProvider
+	| DeezerProvider
+	| iTunesProvider
+	| MoraProvider
+	| MusicBrainzProvider
+	| OtotoyProvider
+	| SpotifyProvider
+	| TidalProvider;


This is the part that gives me the biggest headache... I really want to avoid having to import all provider implementations here, every time a new one is implemented.
The registry should be completely independent from concrete implementations and should only know that these all return HarmonyRelease objects in the end.
That was the whole point behind the inherited abstract releaseLookup property.

Isn't there any way we can have a base class (or interface at least) the registry knows about without having to fight against TypeScript?

I assumed this to be the reason for the design, and I didn't like messing it up with this change.

From my point of view, this design reflects a classic plugin architecture, where you want external contributors to be able to provide a code library that "plugs into" your existing code. In that scenario, it is unreasonable to know all the concrete implementations in advance - it would almost defeat the entire purpose of the plug-in system.

In general, something like that can be implemented, but it can't be combined with class inheritance. You could use interfaces to achieve the same abstraction, but unless all your concrete types come from the same code base, you have no real guarantee about how they will actually behave at runtime, when everything is just JavaScript.

When I was facing this situation, I had to overcome that I was kinda lying to myself and that this abstraction wasn't achieving anything useful. I was already creating the modules in my code base one-by-one, which was the entity management that I was trying to avoid with my abstraction. In the Harmony code base, you also find this:

https://github.com/kellnerd/harmony/blob/main/providers/mod.ts#L23-L33

I had the same type of call in my code, and hated it. But it was really always hinting at a bigger problem.

If you actually do achieve the full abstraction, and you actually interact with external code modules that have providers, you will very quickly run into signature incompatibilities (version conflicts), which are just the next pain down the road.

kellnerd · 2026-03-12T21:24:26Z

server/fresh.gen.ts

 import * as $RegionList from './islands/RegionList.tsx';
 import * as $ReleaseSeeder from './islands/ReleaseSeeder.tsx';
-import { type Manifest } from '$fresh/server.ts';
+import type { Manifest } from '$fresh/server.ts';


This file is auto-generated and shouldn't be changed usually... Did deno fmt suggest this change, and if yes, which version?

Yes, I believe it is the result of deno fmt.

$ deno --version deno 2.7.4 (stable, release, x86_64-unknown-linux-gnu) v8 14.6.202.6-rusty typescript 5.9.2

Maybe another formatter (like Biome) could have been active. It also wasn't clear to me at the time why this change happened.

kellnerd added the meta Refactoring, tests and other tasks label Mar 10, 2026

style: Fix trailing whitespace

ecf5a36

kellnerd requested changes Mar 12, 2026

View reviewed changes

fix: class/instance mixups

795596e

	const releaseResults = await Promise.allSettled(this.queuedReleases.map((_) => _()));
	const releaseResults = await Promise.allSettled(this.queuedReleaseJobs.map((job) => job()));

Conversation

oliversalzburg commented Mar 10, 2026

Uh oh!

kellnerd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants