boardgameio / boardgame.io

State Management and Multiplayer Networking for Turn-Based Games
https://boardgame.io
MIT License
9.99k stars 704 forks source link

Storage API & Logs #577

Closed delucis closed 4 years ago

delucis commented 4 years ago

While implementing the new Firebase connector, I noticed that setState in the flat-file and in-memory implementations handles concatenating logs. I’m not really familiar with what the log is used for, but isn’t this concatenation somewhat more than a storage layer concern?

As far as I can tell, the only place where state is currently persisted that will change log values is in the master during onUpdate. Would it make sense to concatenate logs there and add a setLog storage API method?

nicolodavis commented 4 years ago

The log is used by the Debug Panel to show the user a history of actions in the game. The state object is populated with the entries after processing the action just made.

The main consideration is this: We want the storage implementation to decide how it wants to store the log. For example, the flatfile just uses a single value that stores the entire log (which is why it needs to concatenate it with the previous value). However, a SQL implementation might just append to a table (in which case no concatenation is required on the write path).

If we introduce a setLog, it sounds like the code in master.ts will have to send the entire log to the storage layer every time an action is processed, which is not what we want.

delucis commented 4 years ago

OK, that makes sense. How about making it explicit then? If boardgame.io wants to send the storage API a partial log and have it appended to anything existing, what if the API has an appendLog method? That way the storage API doesn’t have to know that there’s a deltalog key somewhere etc., it just gets handed the deltalog, retrieves the log as necessary and stores the changes:

// master.ts
db.appendLog(gameID, deltalog)

// db implementation
async function appendLog(gameID: string, deltalog: LogEntry[]): Promise<void> {
  // log concatenate or table append etc.
}

Happy to open a PR for this, if it sounds like a reasonable approach.

nicolodavis commented 4 years ago

I like that idea. Feel free to send me a PR. Thanks!