Skip to content

feat: Rewrite module loader #193

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
oliversalzburg wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat: Rewrite module loader #193

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
83ad453
feat: Rewrite module loader
oliversalzburg Mar 10, 2026
ecf5a36
style: Fix trailing whitespace
oliversalzburg Mar 11, 2026
795596e
fix: class/instance mixups
oliversalzburg Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cli.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ if (args._.length === 1) {
let specifier: GTIN | URL = args._[0];

try {
specifier = new URL(specifier);
specifier = new URL(String(specifier));
} catch {
// not a valid URL, treat specifier as GTIN
}
Expand Down
12 changes: 6 additions & 6 deletions lookup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export class CombinedReleaseLookup {
});
return false;
} else {
this.queuedReleases.push(provider.getRelease(id, this.options));
this.queuedReleases.push(() => provider.releaseLookup(id, this.options).getRelease());
this.queuedProviderNames.add(displayName);
this.gtinLookupProviders.delete(provider.internalName);
return true;
Expand All @@ -102,7 +102,7 @@ export class CombinedReleaseLookup {
});
return false;
} else {
this.queuedReleases.push(provider.getRelease(url, this.options));
this.queuedReleases.push(() => provider.releaseLookup(url, this.options).getRelease());
this.queuedProviderNames.add(displayName);
this.gtinLookupProviders.delete(provider.internalName);
return true;
Expand Down Expand Up @@ -143,7 +143,7 @@ export class CombinedReleaseLookup {
const provider = providers.findByName(providerName);
if (provider) {
if (provider.getQuality('GTIN lookup') != FeatureQuality.MISSING) {
this.queuedReleases.push(provider.getRelease(['gtin', this.gtin], this.options));
this.queuedReleases.push(() => provider.releaseLookup(['gtin', gtin.toString()], this.options).getRelease());
this.queuedProviderNames.add(provider.name);
} else {
this.messages.push({
Expand Down Expand Up @@ -183,7 +183,7 @@ export class CombinedReleaseLookup {
return this.cachedReleaseMap;
}

const releaseResults = await Promise.allSettled(this.queuedReleases);
const releaseResults = await Promise.allSettled(this.queuedReleases.map((_) => _()));
Copy link
Copy Markdown
Owner

@kellnerd kellnerd Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()));

Copy link
Copy Markdown
Contributor Author

@oliversalzburg oliversalzburg Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const releasesOrErrors: Array<HarmonyRelease | Error> = await Promise.all(releaseResults.map(async (result) => {
if (result.status === 'fulfilled') {
return result.value;
Expand Down Expand Up @@ -321,7 +321,7 @@ export class CombinedReleaseLookup {
/** Internal names of providers which will be used for GTIN lookups. */
private gtinLookupProviders: Set<string>;

private queuedReleases: Promise<HarmonyRelease>[] = [];
private queuedReleases: Array<() => Promise<HarmonyRelease>> = [];

/** Display names of all queued providers. */
private queuedProviderNames = new Set<string>();
Expand Down Expand Up @@ -349,7 +349,7 @@ export function getReleaseByUrl(url: URL, options?: ReleaseOptions): Promise<Har
throw new LookupError(`No provider supports ${url}`);
}

return matchingProvider.getRelease(url, options);
return matchingProvider.releaseLookup(url, options).getRelease();
}

/**
Expand Down
8 changes: 6 additions & 2 deletions providers/Bandcamp/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ import type {
HarmonyTrack,
Label,
LinkType,
ReleaseOptions,
ReleaseSpecifier,
} from '@/harmonizer/types.ts';
import { type CacheEntry, MetadataProvider, ReleaseLookup } from '@/providers/base.ts';
import { DurationPrecision, FeatureQuality, FeatureQualityMap } from '@/providers/features.ts';
Expand All @@ -22,7 +24,7 @@ import { similarNames } from '@/utils/similarity.ts';
import { toTrackRanges } from '@/utils/tracklist.ts';
import { simplifyName } from 'utils/string/simplify.js';

export default class BandcampProvider extends MetadataProvider {
export default class BandcampProvider extends MetadataProvider<BandcampProvider> {
Copy link
Copy Markdown
Owner

@kellnerd kellnerd Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@oliversalzburg oliversalzburg Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

readonly name = 'Bandcamp';

readonly supportedUrls = new URLPattern({
Expand Down Expand Up @@ -52,7 +54,9 @@ export default class BandcampProvider extends MetadataProvider {
month: 9,
};

readonly releaseLookup = BandcampReleaseLookup;
releaseLookup(specifier: ReleaseSpecifier, options: ReleaseOptions = {}) {
return new BandcampReleaseLookup(this, specifier, options);
}
Comment on lines -55 to +59
Copy link
Copy Markdown
Owner

@kellnerd kellnerd Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


override extractEntityFromUrl(url: URL): EntityId | undefined {
const albumResult = this.supportedUrls.exec(url);
Expand Down
14 changes: 12 additions & 2 deletions providers/Beatport/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
import type { Artist, BeatportNextData, BeatportRelease, Release, Track } from './json_types.ts';
import type { ArtistCreditName, EntityId, HarmonyRelease, HarmonyTrack, LinkType } from '@/harmonizer/types.ts';
import type {
ArtistCreditName,
EntityId,
HarmonyRelease,
HarmonyTrack,
LinkType,
ReleaseOptions,
ReleaseSpecifier,
} from '@/harmonizer/types.ts';
import { variousArtists } from '@/musicbrainz/special_entities.ts';
import { CacheEntry, MetadataProvider, ReleaseLookup } from '@/providers/base.ts';
import { DurationPrecision, FeatureQuality, FeatureQualityMap } from '@/providers/features.ts';
Expand Down Expand Up @@ -30,7 +38,9 @@ export default class BeatportProvider extends MetadataProvider {
recording: 'track',
};

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

override readonly launchDate: PartialDate = {
year: 2005,
Expand Down
4 changes: 3 additions & 1 deletion providers/Deezer/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,9 @@ export default class DeezerProvider extends MetadataApiProvider {

override readonly availableRegions = new Set(availableRegions);

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

override readonly launchDate: PartialDate = {
year: 2007,
Expand Down
6 changes: 5 additions & 1 deletion providers/Mora/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import type {
HarmonyRelease,
HarmonyTrack,
LinkType,
ReleaseOptions,
ReleaseSpecifier,
} from '@/harmonizer/types.ts';
import { type CacheEntry, MetadataProvider, ReleaseLookup } from '@/providers/base.ts';
import { DurationPrecision, FeatureQuality, FeatureQualityMap } from '@/providers/features.ts';
Expand Down Expand Up @@ -49,7 +51,9 @@ export default class MoraProvider extends MetadataProvider {
month: 4,
};

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

constructUrl(entity: EntityId): URL {
return new URL(`https://mora.jp/${entity.type}/${entity.id}/`);
Expand Down
6 changes: 5 additions & 1 deletion providers/MusicBrainz/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import type {
HarmonyTrack,
MediumFormat,
ReleaseGroupType,
ReleaseOptions,
ReleaseSpecifier,
} from '@/harmonizer/types.ts';
import {
type ApiQueryOptions,
Expand Down Expand Up @@ -60,7 +62,9 @@ export default class MusicBrainzProvider extends MetadataApiProvider {
release: 'release',
};

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

readonly apiBaseUrl = musicbrainzApiBaseUrl;

Expand Down
8 changes: 6 additions & 2 deletions providers/Ototoy/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import type {
HarmonyTrack,
Label,
LinkType,
ReleaseOptions,
ReleaseSpecifier,
} from '@/harmonizer/types.ts';
import { type CacheEntry, MetadataProvider, ReleaseLookup } from '@/providers/base.ts';
import { DurationPrecision, FeatureQuality, FeatureQualityMap } from '@/providers/features.ts';
Expand All @@ -16,7 +18,7 @@ import { ProviderError, ResponseError } from '@/utils/errors.ts';
import { DOMParser, HTMLDocument } from '@b-fuze/deno-dom';
import { parseDuration } from '../../utils/time.ts';

export default class OtotoyProvider extends MetadataProvider {
export default class OtotoyProvider extends MetadataProvider<OtotoyProvider> {
readonly name = 'OTOTOY';

readonly supportedUrls = new URLPattern({
Expand Down Expand Up @@ -50,7 +52,9 @@ export default class OtotoyProvider extends MetadataProvider {
month: 8,
};

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

override extractEntityFromUrl(url: URL): EntityId | undefined {
const packageResult = this.supportedUrls.exec(url);
Expand Down
6 changes: 5 additions & 1 deletion providers/Spotify/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ import type {
HarmonyRelease,
HarmonyTrack,
LinkType,
ReleaseOptions,
ReleaseSpecifier,
} from '@/harmonizer/types.ts';

// See https://developer.spotify.com/documentation/web-api
Expand Down Expand Up @@ -66,7 +68,9 @@ export default class SpotifyProvider extends MetadataApiProvider {

override readonly availableRegions = new Set(availableRegions);

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

override readonly launchDate: PartialDate = {
year: 2008,
Expand Down
13 changes: 6 additions & 7 deletions providers/Tidal/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,22 +70,21 @@ export default class TidalProvider extends MetadataApiProvider {

override readonly availableRegions = new Set(availableRegions);

protected releaseLookup: typeof TidalV1ReleaseLookup | typeof TidalV2ReleaseLookup = TidalV2ReleaseLookup;
releaseLookup(specifier: ReleaseSpecifier, options: ReleaseOptions = {}) {
return new TidalV2ReleaseLookup(this, specifier, options);
}

override readonly launchDate: PartialDate = {
year: 2014,
month: 10,
day: 28,
};

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();
Comment on lines -81 to +87
Copy link
Copy Markdown
Owner

@kellnerd kellnerd Mar 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

@oliversalzburg oliversalzburg Mar 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

constructUrl(entity: EntityId): URL {
Expand Down
24 changes: 2 additions & 22 deletions providers/base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ export type MetadataProviderConstructor = new (
* Abstract metadata provider which looks up releases from a specific source.
* Converts the raw metadata into a common representation.
*/
export abstract class MetadataProvider {
export abstract class MetadataProvider<Provider extends MetadataProvider<Provider> | unknown = unknown> {
constructor({
rateLimitInterval = null,
concurrentRequests = 1,
Expand Down Expand Up @@ -104,23 +104,11 @@ export abstract class MetadataProvider {
/** Maps MusicBrainz entity types to the corresponding entity types of the provider. */
abstract readonly entityTypeMap: Record<HarmonyEntityType, string | string[]>;

protected abstract releaseLookup: ReleaseLookupConstructor;

/** Country codes of regions in which the provider offers its services (optional). */
readonly availableRegions?: Set<CountryCode>;

readonly launchDate: PartialDate = {};

/** Looks up the release which is identified by the given specifier (URL, GTIN/barcode or provider ID). */
getRelease(specifier: ReleaseSpecifier, options: ReleaseOptions = {}): Promise<HarmonyRelease> {
try {
const lookup = new this.releaseLookup(this, specifier, options);
return lookup.getRelease();
} catch (error) {
return Promise.reject(error);
}
}

/** Checks whether the provider supports the domain of the given URL. */
supportsDomain(url: URL | string): boolean {
return new URLPattern({ hostname: this.supportedUrls.hostname }).test(url);
Expand Down Expand Up @@ -322,15 +310,7 @@ export abstract class MetadataProvider {
}
}

type ReleaseLookupConstructor = new (
// It is probably impossible to specify the correct provider subclass here.
// deno-lint-ignore no-explicit-any
provider: any,
specifier: ReleaseSpecifier,
options: ReleaseOptions,
) => ReleaseLookup<MetadataProvider, unknown>;

export abstract class ReleaseLookup<Provider extends MetadataProvider, RawRelease> {
export abstract class ReleaseLookup<Provider extends MetadataProvider<Provider>, RawRelease> {
/** Initializes the release lookup for the given release specifier. */
constructor(
protected provider: Provider,
Expand Down
6 changes: 5 additions & 1 deletion providers/iTunes/mod.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ import type {
HarmonyRelease,
LinkType,
ReleaseGroupType,
ReleaseOptions,
ReleaseSpecifier,
} from '@/harmonizer/types.ts';

// See https://developer.apple.com/library/archive/documentation/AudioVideo/Conceptual/iTuneSearchAPI
Expand Down Expand Up @@ -44,7 +46,9 @@ export default class iTunesProvider extends MetadataApiProvider {

override readonly availableRegions = new Set(availableRegions);

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

override readonly launchDate: PartialDate = {
year: 2003,
Expand Down
Loading