Decouple download provider from full list of ThunderstoreMod objects #1482

anttimaki · 2024-10-11T10:49:37Z

No description provided.

ebkr · 2024-10-11T17:24:36Z

src/r2mm/manager/PackageDexieStore.ts

+ * @returns ThunderstoreCombo[], silently omitting unknown packages and versions.
+ */
+export async function getCombosByDependencyStrings(
+    community: string,


It would be better for this to take a Game type and for this method to handle how to extract that. I don't think that the download provider should need to know that an internal folder name is what drives the community identifier.

Makes sense.

For the background, when I whipped up the Dexie schema, I would've preferred to use the same community slug as is used in Thunderstore, but it wasn't available (aside from stripping from the API URL). I didn't want to add a new field to the GameManager mega list which already contains various formats of the spelling the game's name, so I just picked one that seemed most suitable. So although the variable is named "internalFolderName", what I use it for is "community identifier".

Yeah I'm happy for it to be the community identifier, it's mostly just to separate functional concerns

Changed to accept Game object instead of id string.

src/r2mm/manager/PackageDexieStore.ts

ebkr · 2024-10-11T17:32:04Z

src/r2mm/manager/PackageDexieStore.ts

+        combo.setMod(ThunderstoreMod.parseFromThunderstoreData(rawPackage));
+        combo.setVersion(ThunderstoreVersion.parseFromThunderstoreData(rawVersion));
+        return combo;
+    }).filter((c): c is ThunderstoreCombo => c !== undefined);


Two points on this:

Not a huge fan of this syntax. Do we need to use (c): c is ThunderstoreCombo instead of (c: ThunderstoreCombo?) => c !== undefined

Do we need to cast it? c => c !== undefined) is arguably more concise and gets the point across clearer. There may be an ESLint rule blocking this, so I'm happy for that to be the answer.

If neither of these strike you too much, I'm also not opposed to a util method to simply change this to something like: .filter(ObjectUtils.onlyDefinedValues)

It's not about ESLint. Does the repo even use ESLint?

Anyway, semantically speaking, I think the user-defined type guard is considered "type narrowing", and not brute force "type casting". AFAIK using "x is y" type guard functions is perfectly fine and TypeScriptic way of doing things like this.

In this particular case it's optional (due to simplicity of the case), and can be left out. At the same time the code base uses things like private packageUrl: string = ''; where the string part is fully optional and not needed by TypeScript. But someone has considered it clearer to include these extra type descriptions only for the readers benefit. I don't think using the type guard here is any different. And when discussion is based on personal preferences, the proper solution would be to introduce linting to automate forcing the conventions instead of discussing them in code reviews. But that's outside the scope.

It's not about ESLint. Does the repo even use ESLint?

Potentially not. It did at one point and I think my configuration still has remnants of it.

Anyway, semantically speaking, I think the user-defined type guard is considered "type narrowing", and not brute force "type casting". AFAIK using "x is y" type guard functions is perfectly fine and TypeScriptic way of doing things like this.

I'm fine with type guards where we actually need them, I don't think we've really used them much on the project and they don't appear to really add any benefit here as we don't care about the type of the filter. Supposedly we can do

.filter<ThunderstoreCombo>(c => c !== undefined)

which might read a bit nicer?

At the same time the code base uses things like private packageUrl: string = ''; where the string part is fully optional and not needed by TypeScript

That looks like remnants of ESLint, granted I do prefer type declarations on properties themselves.

.filter<ThunderstoreCombo>(c => c !== undefined)

Fails for me on dev server build: TS2345: Argument of type '(c: ThunderstoreCombo | undefined) => boolean' is not assignable to parameter of type '(value: ThunderstoreCombo | undefined, index: number, array: (ThunderstoreCombo | undefined)[]) => value is ThunderstoreCombo'.

Did not change.

ebkr · 2024-10-11T17:38:10Z

src/utils/DependencyUtils.ts

+    const parts = dependencyString.split('-');
+
+    if (parts.length !== 3) {
+        throw new Error(`Invalid dependency string "${dependencyString}"`);


Careful, this is invalid. Author names can have hyphens iirc.

You can assume the dependency string IS valid because we're supplied it by TS. If a user attempts to import a local mod and gets it wrong then we just won't resolve it.

You can instead do (pseudo):

splits = dependencyString.split('-'); splits.last() => version splits.secondLast() => mod name splits.rest().join("-") => author name

Right. I guess I assumed that because database doesn't currently (and haven't in almost 4 years) accept hyphens in team names, there would be no hyphens in team names. Do we actually know the same isn't true for package names?

That'd be one for @MythicManiac

Changed the implementation to accept more than two hyphens.

ebkr · 2024-10-11T17:40:13Z

src/r2mm/downloading/BetterThunderstoreDownloader.ts

+        await this._buildDependencySet(mod, builder, useLatestVersion);
+    }
+
+    private async _buildDependencySet(mod: ThunderstoreVersion, builder: ThunderstoreCombo[], useLatestVersion=false): Promise<void> {


This is more of a preference thing but it would be nicer to not have this defaulted. It could be more readable to create an Enum and we can utilise DependencySetBuilderStyle.USE_LATEST as a more readable equivalent wherever needed.

Default behaviour is weird IMO and we'll just prevent issues by being explicit.

Generally speaking I don't consider parameter default values a negative thing. Of course one can go overboard with them, and e.g. using mutable default values is a trap. Constants and enums also have their use, especially to avoid typos on magic strings. I'm not sure how well that applies to booleans though.

In this particular case, we're talking about a private method, which is only called by the two named wrapper methods above. So I'd argue that having the named methods is more readable than having _buildDependencySet use enum parameter. (Also the named methods are public although they're only used internally, so to get rid of them would require changing the interface. Which might not be a bad thing in itself, but is a bit of scope creep.)

The problem with default values is that it's extremely easy to miss them out. I'd much rather we were explicit with our signatures.

A boolean is IMO still a magic variable.
If you were to see the following on its own with no other context:

_buildDependencySet(mod, dependencies, true)

The true value doesn't really mean anything.

Equally, if you were to see:

_buildDependencySet(mod, dependencies, true) ... _buildDependencySet(mod, dependencies) ... _buildDependencySet(mod, dependencies, false)

You don't really know what's being done differently without context of the parameter's name

Yeah I understood the point but don't personally see it as a much of an issue. Addressed this in a separate commit.

The helper is used to convert an array of dependency strings to ThunderstoreCombo objects using data from the IndexedDB. This is a preliminary step for changes to reduce the need to have a full list of community's mods and their versions in memory at all times. read the data required to build an arrayy dependency tree (well, an array at least

Reading the data from DB allows eventually dropping the reference to whole list of ThunderstoreMods passed as an argument. Unfortunately this requires bringing in a dependency to GameManager. It's required currently to know which community's package list should be used to look for the mods. Ideally the packages and listings would be separated, but that's outside of the scope of the current task. Additionally it would change the behaviour, as currently inter-community dependencies aren't allowed.

…edDB Reading the data from DB allows eventually dropping the reference to whole list of ThunderstoreMods passed as an argument. Unfortunately this requires bringing in a dependency to GameManager. It's required currently to know which community's package list should be used to look for the mods. Ideally the packages and listings would be separated, but that's outside of the scope of the current task. Additionally it would change the behaviour, as currently inter-community dependencies aren't allowed.

None of the provider's methods no longer need to have access to full list of mods available on Thunderstore API. The same data is read directly from IndexedDB when it's needed.

The result array was both returned from the function, but also mutated in place. I assume this is done to avoid having to create multiple copies when the callstack gets deep. Stop returning the array to make it clear it's used by reference.

Default parameters are considered harmful as someone might forget to use them. Required Enum parameters are considered better for readability.

ebkr requested changes Oct 11, 2024

View reviewed changes

Base automatically changed from import-progress-details to develop October 14, 2024 12:01

anttimaki added 7 commits October 17, 2024 10:49

ThunderstoreDownloaderProvider: drop dependency on allMods

3e2f750

None of the provider's methods no longer need to have access to full list of mods available on Thunderstore API. The same data is read directly from IndexedDB when it's needed.

Reduce code duplication

a7813cd

Change ThunderstoreDownloaderProvider based on review feedback

04be4c5

Default parameters are considered harmful as someone might forget to use them. Required Enum parameters are considered better for readability.

anttimaki force-pushed the memory-optimization-pt1-DownloadProvider branch from 80b3cbf to 04be4c5 Compare October 17, 2024 09:36

anttimaki requested a review from ebkr October 17, 2024 09:46

ebkr approved these changes Oct 18, 2024

View reviewed changes

anttimaki merged commit b1936a4 into develop Oct 18, 2024
5 checks passed

anttimaki deleted the memory-optimization-pt1-DownloadProvider branch October 18, 2024 08:57

Decouple download provider from full list of ThunderstoreMod objects #1482

Decouple download provider from full list of ThunderstoreMod objects #1482

Uh oh!

Conversation

anttimaki commented Oct 11, 2024

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!

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!

ebkr Oct 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Uh oh!

ebkr Oct 16, 2024 •

edited

Loading