WIP: Callback stack refactoring and installation after download #1564
Conversation
There was a problem hiding this comment.
NGL making out what's going on here was difficult and I'd have better confidence regarding my comments if this stuff was implemented piecemeal in multiple commits. Also I didn't test this locally as this and the previous PR have outstanding issues that will likely change things quite a bit.
| @@ -145,6 +154,10 @@ let assignId = 0; | |||
| return this.$store.getters['modDownload/activeDownloadModName']; | |||
| } | |||
|
|
|||
| get activeInstallProgress(): number { | |||
There was a problem hiding this comment.
The Vuex implementation may return undefined as well, so using number as the return type here is a bug.
| ignoreCache: boolean, | ||
| callback: (progress: number, modName: string, status: number, err: R2Error | null) => void, | ||
| completedCallback: (modList: ThunderstoreCombo[]) => void): void; | ||
| public abstract download( |
There was a problem hiding this comment.
- We could consider renaming this to "downloadWithDependencies" oslt
- Depending on the callsites we might take in the main mod as a ThunderstoreCombo (I think we currently keep splitting and combining the mod & version unnecessarily in different places)
| callback: (progress: number, modName: string, status: number, err: R2Error | null) => void, | ||
| completedCallback: (modList: ThunderstoreCombo[]) => void) { | ||
|
|
||
| public async download( |
There was a problem hiding this comment.
These comments are based on just looking at the code so there's quite a change I'm missing something, but the approach taken here doesn't seem optimal to me. I would rather:
- Gather everything we're going to download (preferably into one array, but there might be a reason to handle the "main download" separately from the dependencies). Pretty much as the code currently does, although there might be some changes to clean up / simplify the implementation
- Instead of using queueDownloadDependencies (even a refactored one) use
_downloadComboand_saveDownloadResponsehelpers likedownloadImportedMods. I think the queueing makes the code harder to read.
| 'updateDownloadProgress', | ||
| { progress: 100, modName: mod.getMod().getName(), status: StatusEnum.SUCCESS, err: null } | ||
| ); | ||
| await dispatch('installModAfterDownload', { profile: params.profile, mod: params.mod }).then(() => { |
There was a problem hiding this comment.
IIRC ProfileUtils.installModsToProfile should be usable here. The implementation in this PR (copy pasted from Vue component?) is inefficient as it does multiple unnecessary disk operations for each mod. Don't take my word for it though; check and make sure the implementations actually do all the same stuff.
VilppeRiskidev
force-pushed
the
download-vuex-poc
branch
from
c096a27
to
c7896a4
Compare
https://github.com/thunderstore-io/thunderstore-mod-manager/pull/285