Skip to content
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

Remove any-types and Mod class #1204

Merged
merged 2 commits into from
Feb 15, 2024
Merged

Remove any-types and Mod class #1204

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d2a2be6
Remove untyped arguments from LocalModList
anttimaki Feb 6, 2024
bbd7ea6
Remove Mod class
anttimaki Feb 7, 2024
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
58 changes: 23 additions & 35 deletions src/components/views/LocalModList.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ import ProfileModList from '../../r2mm/mods/ProfileModList';
import R2Error from '../../model/errors/R2Error';
import ManagerSettings from '../../r2mm/manager/ManagerSettings';
import ModBridge from '../../r2mm/mods/ModBridge';
import Mod from '../../model/Mod';
import DependencyListDisplayType from '../../model/enums/DependencyListDisplayType';
import Dependants from '../../r2mm/mods/Dependants';
import ProfileInstallerProvider from '../../providers/ror2/installing/ProfileInstallerProvider';
Expand Down Expand Up @@ -142,8 +141,7 @@ import SearchAndSort from './LocalModList/SearchAndSort.vue';
}
}

getMissingDependencies(vueMod: any): string[] {
const mod: Mod = new Mod().fromReactive(vueMod);
Copy link
Owner

Choose a reason for hiding this comment

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

Do we have any issues from having dropped the fromReactive call?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My next performance improvement branch is based on this branch, so I've been monkey testing it quite lot, and so far I've not seen any problems in (dev) building the project, errors logged in the console, or weird behaviour in the UI itself. But work on that continues so I still have more changes to observe these changes as well.

However, I'm not entirely sure in what situations should we expect the objects to get serialized and cause problems in the first place?

Copy link
Owner

Choose a reason for hiding this comment

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

It's related to loading the profile from the mods.yml file. ManifestV2 is serialized whenever ProfileModList is called to save, and is deserialized whenever we load the contents again

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is probably good to double check, and if it does prove to still be a potential issue, in my opinion the proper deserialization should simply be moved to be a part of the loading process.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this should not be an issue, the source of the data seems to all trace back to ProfileModList.getModList which also instantiates a proper object for it rather than just passing in the values. This change just simply adds the proper type annotation here and in doing so makes it apparent that the Mod class is not actually needed anymore.

It is possible that the vuex state somehow ends up containing something other than it's annotated to do without our current TS configuration catching it (or e.g. due to type casting that should not be done somewhere), but I couldn't find any such code at least (as mentioned, most seems to originate from ProfileModList.getModList, or alternatively some of the mutation operations in the same module)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Alright so the main difference here is that the Mod.fromReactive implementation uses the actual package manifest.json for populating the values, whereas the ProfileModList.getModList uses whatever happens to be populated in the mods.yml. In a sense the Mod.fromReactive implementation behaves more "correctly" in this case as it's actually using the data provided by the package itself rather than a state cache file.

In practice I'm not sure if it matters, but I do think the correct way to load mod metadata is to read it from said mod's own manifest file. I suppose the changes in this PR are still an improvement because it's better to have the metadata parsing be consistent and predictable, which is being achieved by removing the rarely used alternative metadata parsing.

getMissingDependencies(mod: ManifestV2): string[] {
return mod.getDependencies().filter((dependency: string) => {
// Include in filter if mod isn't found.
return this.$store.state.localModList.find(
Expand All @@ -152,9 +150,8 @@ import SearchAndSort from './LocalModList/SearchAndSort.vue';
});
}

getDisabledDependencies(vueMod: any): ManifestV2[] {
const dependencies = new Mod()
.fromReactive(vueMod)
getDisabledDependencies(mod: ManifestV2): ManifestV2[] {
const dependencies = mod
.getDependencies()
.map((x) => x.toLowerCase().substring(0, x.lastIndexOf('-') + 1));

Expand Down Expand Up @@ -192,13 +189,11 @@ import SearchAndSort from './LocalModList/SearchAndSort.vue';
return modList;
}

async disableModWithDependents(vueMod: any) {
const mod: ManifestV2 = new ManifestV2().fromReactive(vueMod);
async disableModWithDependents(mod: ManifestV2) {
await this.disableMods([...this.getDependantList(mod), mod]);
}

async disableModExcludeDependents(vueMod: any) {
const mod: ManifestV2 = new ManifestV2().fromReactive(vueMod);
async disableModExcludeDependents(mod: ManifestV2) {
await this.disableMods([mod]);
}

Expand Down Expand Up @@ -247,13 +242,11 @@ import SearchAndSort from './LocalModList/SearchAndSort.vue';
await this.updateModListAfterChange(updatedList);
}

async uninstallModWithDependents(vueMod: any) {
let mod: ManifestV2 = new ManifestV2().fromReactive(vueMod);
async uninstallModWithDependents(mod: ManifestV2) {
await this.uninstallMods([...this.getDependantList(mod), mod]);
}

async uninstallModExcludeDependents(vueMod: any) {
let mod: ManifestV2 = new ManifestV2().fromReactive(vueMod);
async uninstallModExcludeDependents(mod: ManifestV2) {
await this.uninstallMods([mod]);
}

Expand Down Expand Up @@ -291,23 +284,21 @@ import SearchAndSort from './LocalModList/SearchAndSort.vue';
await this.updateModListAfterChange(result);
}

showDependencyList(vueMod: any, displayType: string) {
this.selectedManifestMod = new ManifestV2().fromReactive(vueMod);
showDependencyList(mod: ManifestV2, displayType: string) {
this.selectedManifestMod = mod;
this.dependencyListDisplayType = displayType;
this.showingDependencyList = true;
}

uninstallModRequireConfirmation(vueMod: any) {
const mod: ManifestV2 = new ManifestV2().fromReactive(vueMod);
uninstallModRequireConfirmation(mod: ManifestV2) {
if (this.getDependantList(mod).size === 0) {
this.performUninstallMod(mod);
} else {
this.showDependencyList(mod, DependencyListDisplayType.UNINSTALL);
}
}

disableModRequireConfirmation(vueMod: any) {
const mod: ManifestV2 = new ManifestV2().fromReactive(vueMod);
disableModRequireConfirmation(mod: ManifestV2) {
for (const value of this.getDependantList(mod)) {
if (value.isEnabled()) {
this.showDependencyList(mod, DependencyListDisplayType.DISABLE);
Expand All @@ -317,13 +308,11 @@ import SearchAndSort from './LocalModList/SearchAndSort.vue';
this.performDisable([mod]);
}

viewDependencyList(vueMod: any) {
const mod: ManifestV2 = new ManifestV2().fromReactive(vueMod);
viewDependencyList(mod: ManifestV2) {
this.showDependencyList(mod, DependencyListDisplayType.VIEW);
}

async enableMod(vueMod: any) {
const mod: ManifestV2 = new ManifestV2().fromReactive(vueMod);
async enableMod(mod: ManifestV2) {
try {
const result = await this.performEnable([...this.getDependencyList(mod), mod]);
if (result instanceof R2Error) {
Expand Down Expand Up @@ -359,23 +348,22 @@ import SearchAndSort from './LocalModList/SearchAndSort.vue';
await this.updateModListAfterChange(updatedList);
}

updateMod(vueMod: any) {
this.selectedManifestMod = new ManifestV2().fromReactive(vueMod);
const mod = ModBridge.getCachedThunderstoreModFromMod(
this.selectedManifestMod
);
if (mod instanceof ThunderstoreMod) {
this.$store.commit("openDownloadModModal", mod);
updateMod(mod: ManifestV2) {
this.selectedManifestMod = mod;
const tsMod = ModBridge.getCachedThunderstoreModFromMod(mod);

if (tsMod instanceof ThunderstoreMod) {
this.$store.commit("openDownloadModModal", tsMod);
} else {
this.$store.commit("closeDownloadModModal");
}
}

downloadDependency(missingDependency: string) {
const mod: ThunderstoreMod | undefined = this.thunderstorePackages.find(
(tsMod: ThunderstoreMod) => missingDependency.toLowerCase().startsWith(tsMod.getFullName().toLowerCase() + "-")
const tsMod: ThunderstoreMod | undefined = this.thunderstorePackages.find(
(m: ThunderstoreMod) => missingDependency.toLowerCase().startsWith(m.getFullName().toLowerCase() + "-")
);
if (mod === undefined) {
if (tsMod === undefined) {
this.$store.commit("closeDownloadModModal");
const error = new R2Error(
`${missingDependency} could not be found`,
Expand All @@ -385,7 +373,7 @@ import SearchAndSort from './LocalModList/SearchAndSort.vue';
this.$emit('error', error);
return;
}
this.$store.commit("openDownloadModModal", mod);
this.$store.commit("openDownloadModModal", tsMod);
}

async created() {
Expand Down
90 changes: 0 additions & 90 deletions src/model/Mod.ts
View file
Open in desktop

This file was deleted.

78 changes: 75 additions & 3 deletions src/model/ThunderstoreVersion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,16 @@
import Mod from './Mod';
import VersionNumber from './VersionNumber';
import ReactiveObjectConverterInterface from './safety/ReactiveObjectConverter';
import CdnProvider from '../providers/generic/connection/CdnProvider';

export default class ThunderstoreVersion extends Mod implements ReactiveObjectConverterInterface {
export default class ThunderstoreVersion implements ReactiveObjectConverterInterface {

private name: string = '';
private versionNumber: VersionNumber = new VersionNumber('0.0.0');
private dependencies: string[] = [];
private fullName: string = '';
private description: string = ''
private icon: string = ''
private enabled: boolean = true;
private downloads: number = 0;
private downloadUrl: string = '';

Expand All @@ -21,12 +27,78 @@ export default class ThunderstoreVersion extends Mod implements ReactiveObjectCo
}

public fromReactive(reactive: any): ThunderstoreVersion {
super.fromReactive(reactive);
this.setName(reactive.name);
this.setVersionNumber(new VersionNumber('0.0.0').fromReactive(reactive.versionNumber));
this.setDependencies(reactive.dependencies);
this.setFullName(reactive.fullName);
this.setDescription(reactive.description);
this.setIcon(reactive.icon);
this.enabled = reactive.enabled;
this.setDownloadCount(reactive.downloadCount);
this.setDownloadUrl(reactive.downloadUrl);
return this;
}

public getName(): string {
return this.name;
}

public setName(name: string) {
this.name = name;
}

public getVersionNumber(): VersionNumber {
return this.versionNumber;
}

public setVersionNumber(versionNumber: VersionNumber) {
this.versionNumber = versionNumber;
}

public getDependencies(): string[] {
return this.dependencies;
}

public setDependencies(dependencies: string[]) {
this.dependencies = dependencies;
}

public getFullName(): string {
return this.fullName;
}

public setFullName(name: string) {
this.fullName = name;
}

public getDescription(): string {
return this.description;
}

public setDescription(description: string) {
this.description = description;
}

public getIcon(): string {
return this.icon;
}

public setIcon(icon: string) {
this.icon = icon;
}

public isEnabled(): boolean {
return this.enabled;
}

public enable() {
this.enabled = true;
}

public disable() {
this.enabled = false;
}

public getDownloadCount(): number {
return this.downloads;
}
Expand Down
59 changes: 0 additions & 59 deletions src/r2mm/mods/ModFromManifest.ts
View file
Open in desktop

This file was deleted.

Loading