Eranziel / foundryvtt-lancer

A Foundry VTT game system for LANCER RPG.
GNU General Public License v3.0
105 stars 63 forks source link

[RFC] Remove COMP/CON Authentication #706

Open BoltsJ opened 3 months ago

BoltsJ commented 3 months ago

Proposed removal of COMP/CON login for pilot importing

Reasons we might want to remove this feature

Reasons we would want to keep this feature

lazulit3 commented 1 month ago

Your rationale / considerations all seem reasonable to me.

I am in favor of keeping the COMP/CON integration and find the pilot import features useful (though I should probably try setting up a pilot/mech directly in Foundry to get a feel for the hypothetical alternative workflow w/o import).

Maintenance: Keeping the aws dependencies updated represents a non-trivial amount of work.

I am willing to assist with the maintenance work around this.

https://github.com/Eranziel/foundryvtt-lancer/pull/756 bumps aws-amplify to major version 5, but v6 is a more significant movement.

If getting aws-amplify on v6 is desired, Iet me know and I'm willing to do some discovery/exploration/testing around this (and implementation if I don't hit a wall).

Security: There are several current security notices for this repo due to aws-amplify and its dependencies.

I believe the current vulns are addressed by above PR (but I recognize that if more pop up with aws-amplify v5, resolving that would mean dealing with v5 -> v6 upgrade path).

Polish: The current implementation has some quirks. For example on my machine/account, every pilot I have ever made and synced is listed even though most have been deleted.

I tried to reproduce this a little without success. I may not be following the same steps as you, or I wonder if this could do with quirks around syncing deletions to cloud.

If you would like to open an issue with specific steps for reproducing this, I could take a look and try to determine whether this is an issue with COMP/CON or the system's integration. That does sound like a real nuisance.

Performance: Removal of the aws code results in 1MB reduction in the js bundle, which is about 1/3 smaller than the current bundle.

Out of curiosity, how do we measure this? Does "bundle" refer to the total size of the built contents of dist/ ?


Anyway, I don't think this proposal is unreasonable. Am willing to try to help with pain points around COMP/CON integration to the extent that I can. Feel free to point me at what you consider a priority.

BoltsJ commented 1 month ago

Amplify 5 has at least one vulnerable dependency (axios), so 6 is preferred to 5. The issue with old data being retained doesn't happen to everyone, but it's still a pretty common issue. I went in a checked, and while a lot of old stuff was gone, creating then deleting a few pilots, including forcing immediate deletion, left me with several pilots in the dropdown that aren't part of my compcon data. This is especially annoying if you end up with duplicate entries. Even if it doesn't affect everyone, this is a significant ux problem. image image