Skip to content

Conversation

anttimaki
Copy link
Collaborator

No description provided.

V2 extends the original PackageInstaller by adding support for
installer specific uninstallation logic.

Having two separate versions is a bit hacky and requires the profile
installer provider to do some extra checks. However, this allows us to
add support for one installer at the time, instead of having to
implement them all at once.
@anttimaki
Copy link
Collaborator Author

Marked as draft because:

  • Need to discuss with Mythic if the PackageInstallerV2 approach is a desired one
  • Need to verify with the community that I understood correctly the desired changes to (un)installation logic

Uninstalling a mod using ReturnofModding should remove the plugins_data
subdir. However, if it contains a cache subdir, the contents of that
subdir are kept, while rest of the content is removed.

This is done as a way to persist some of the data created by the plugin
when the mod is updated, since the update process uninstalls the old
version before the new one is installed. The behaviour is documented at
https://github.com/xiaoxiao921/ReturnOfModdingBase/tree/master?tab=readme-ov-file#plugins_data
- Actually rename the leaf item of the path
- Throw error if source file/dir and target path don't exists to be in
  line with Node's rename()
- Throw error if attempting to rename an item at the root of mocked
  file system. Rename (and other methods) assume there's some form of
  dir structure. Fixing this would be too much work now, so just
  document the "feature"
- Changing the state of the mod loader does nothing, which is in line
  with what BepInEx does, so I guess it's okay
- Disabling a mod renames the plugin's files to have .old file
  extension and enabling a mod undoes this
- The change also affects games using InstallRuleInstaller with the
  SUBDIR_NO_FLATTEN tracking method, making it match the functionality
  SUBDIR tracking method already had
@MythicManiac
Copy link
Collaborator

Somewhat related to the comment here: #1388 (comment), I think the best approach would be to add optional methods to the current interface (so type hint it as potentially being undefined) rather than creating a second interface. Reasoning being that it's less code changes required elsewhere in the codebase and easier to upgrade the optional methods to required methods once we feel everything is ready for it (just changing the type definition suffices, type checks then verify everything works as intended).

Based on code review feedback, implement the uninstallation methods as
optional methods, rather than have a separate version of the abstract
class with more abstract methods.

Replace the approach with abstract classes and methods with a plain
TypeScript interface because having optional methods in abstract
classes seems poorly supported in TypeScript.
@anttimaki anttimaki changed the title Add PackageInstallerV2 and apply it to ReturnOfModding Support custom uninstallation methods and implement it for ReturnOfModding Aug 20, 2024
@anttimaki
Copy link
Collaborator Author

Somewhat related to the comment here: #1388 (comment), I think the best approach would be to add optional methods to the current interface (so type hint it as potentially being undefined) rather than creating a second interface. Reasoning being that it's less code changes required elsewhere in the codebase and easier to upgrade the optional methods to required methods once we feel everything is ready for it (just changing the type definition suffices, type checks then verify everything works as intended).

@MythicManiac changes added in a separate commit 280852c. Note that this drops the abstract class approach since TypeScript seems to support optional methods in abstract classes poorly.

Base automatically changed from rom-plugin-installer to develop September 2, 2024 06:32
@anttimaki anttimaki merged commit 5fb2f45 into develop Sep 2, 2024
5 of 7 checks passed
@anttimaki anttimaki deleted the rom-installation-changes branch September 2, 2024 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants