Drop ThunderstoreVersion references from ThunderstoreMod #1487
Conversation
|
|
||
| public getVersions(): ThunderstoreVersion[] { | ||
| return this.versions; | ||
| public getLatestVersion(): string { |
There was a problem hiding this comment.
I think we should decide on only having either this or the VersionNumber getter. Not a fan of having both. I'd lean more towards the VersionNumber one if we can get away with it although as per the other comment, it would be nicer to not have to create an instance of the VersionNumber every time it's requested
There was a problem hiding this comment.
Ended up dropping getLatestVersionNumber as it was used only in one place.
| private categories: string[] = []; | ||
| private hasNsfwContent: boolean = false; | ||
| private donationLink: string | undefined; | ||
| private latestVersion: string = ''; |
There was a problem hiding this comment.
Do we have much overhead by converting this to a VersionNumber type?
There was a problem hiding this comment.
I didn't run any benchmarking, but whatever is the difference between the object and the string, will necessarily have some sort of impact after it gets repeated to 37k packages. As the goal of the changes is to reduce memory usage, less is more?
| return this; | ||
| } | ||
|
|
||
| public fromReactive(reactive: any): ThunderstoreVersion { |
There was a problem hiding this comment.
The reason for this previously was because when this was deserialized it lost any prototype information it had attached (EG: methods).
Is this not needed anymore?
There was a problem hiding this comment.
At least the OnlineModList, which was the only place where this was used, works without a hitch, method calls and all.
| version.setFullName(`${mod}-1.0.0`); | ||
| version.setDependencies(dependencyNames.map((depName) => `${depName}-1.0.0`)); | ||
| mod.setVersions([version]); | ||
| // TODO: ThunderstoreMod no longer carries information about versions. |
There was a problem hiding this comment.
We have TODOs written however no further tickets being worked on as part of this work.
If this isn't relevant and will not be worked on for the foreseeable future then we can remove the TODOs.
If you have tickets to work on this at a later point, I'd add the ticket reference in some format of:
// TODO (TS) REF/123: Description of what to do
Reasoning for this is that a TODO in the code adds extremely little value and just won't get done.
There was a problem hiding this comment.
Removed the TODO comments added in this PR
anttimaki
force-pushed
the
memory-optimization-pt5-TsModsModule
branch
from
4be3ab5 to
8f354f5
Compare
anttimaki
force-pushed
the
memory-optimization-pt6-ThunderstoreMod
branch
from
3662fdf to
2139aa9
Compare
As far as I can tell, the conversion isn't really needed. The doc strings of ReactiveObjectConverterInterface claims that access to class methods is lost, but based on testing this is not true. Another claim is that this grants access to object's private fields. The fields can be accessed, but same is true to any ThunderstoreMod object, even if they aren't passed through Vue's methods. Aside from getting a clear TypeScript warning about accessing a private field in IDE (compile time) nothing prevents accessing the field in JS files (run time). Ideally this might speed up OnlineModList rendering, as the unnecessary object creation is skipped.
Commonly used version-specific data - latest version number, icon and description are now stored in the ThunderstoreMod object. Rest of the version info needs to be fetched separately where it's actually used. This change drastically reduces memory usage for larger communities like Lethal Company.
anttimaki
force-pushed
the
memory-optimization-pt5-TsModsModule
branch
from
8f354f5 to
b092517
Compare
anttimaki
force-pushed
the
memory-optimization-pt6-ThunderstoreMod
branch
from
2139aa9 to
4c2db6c
Compare
No description provided.