-
Notifications
You must be signed in to change notification settings - Fork 277
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.
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
CODEBASE: Validate AllGangs and StockMarket after loading with JSON.parse #1783
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very exciting! I wasn't expecting you (or anyone) to take this on, which we'll need to be really careful with because it has lots of implications for old saves.
AllGangs
If we add or move gangs in the future, we have to refactor our code to deal with missing gangs and unknown gangs. Loading a newer save file with a different gang list in our current code results in bugs. StockMarketI removed many requirements as you requested. Tested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great! Aside from the minor business with comments, this is just waiting on me testing it against old saves.
…tockMarket-after-loading-with-JSON.parse
Ideally, when we validate loaded data from JSON.parse, we should validate the actual data (checking the data with JSON schemas), not just the "shape" of it (using instanceOf, checking if it's an object with typeof, etc.). Doing that for all json-able classes requires a tremendous effort while the benefit is too small (invalid data indicates buggy saving/loading code or badly edited save file). That's the reason why we have not validated the actual data until now. Originally, I only planned to validate theme data, because it can be set directly by the player via UI. In #1775, I see that AllGangs and StockMarket can also be validated this way, so I made 4 exceptions for that PR:
This PR handles AllGangs and StockMarket.
Test:
AllGangs:
AllGangs
's structure has not been changed since at least 9137c24 (v0.43.1). I tested with a save file from v1.0.0.StockMarket:
StockMarket
's structure has not been changed since 1b41e33 (this commit was included in v1.3.0). I tested with a save file from v1.3.0.Webpack Bundle Analyzer: