Skip to content
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

Initial proof of concept db #383

Closed
wants to merge 1 commit into from
Closed

Conversation

agiera
Copy link

@agiera agiera commented Jun 19, 2023

It uses TypeORM. I opted to go with that one because it supports enum. So there's really minimal changes made to the ipc logic currently. But I think it's a good start that keeps things flexible. It still needs to support player stats before it's PR ready, but I wanted to check if this is the direction we want to go before investing more time.

  1. One thing to note is that I have the sqlite db live in the replays folder. I'm not sure where a good place for it is. It might be nice to have it in replays so that if you copy slippi replay dirs to another computer the cache still works. It doesn't currently update without a restart though.

  2. TypeORM doesn't handle nested queries as well as I hoped it would. I might try rewriting this solution using prisma and just use Int as the type instead of enum. Hopefully that will work out of the box instead of casting.

  3. Another question that was brought up is if we should use a query builder like https://github.com/kysely-org/kysely instead. I'm not sure I understand the advantages. Is it easier to maintain?

  4. Some things I think this solution needs before merging are:
    a. Filtering the filenames by whether their in the db and then doing a bulk insert for the ones that aren't in there. Right now it's using bad form and just spamming queries.
    b. Support for player stats (create entity and integrate into ipc code)
    c. Deciding on where to put db file. If it's staying with replays we should change the filename to .index.sqlite3 so it's hidden and also use relative paths as the primary key.

Is there anything I'm missing?

  1. Some things that this change enables and could be added after a merge are:
    a. Modify UI to use ORM and change workers to return void
    b. Implement infinite scrolling to avoid loading all data at once
    c. Global stats
    d. ect.

@vinceau vinceau mentioned this pull request Oct 28, 2023
5 tasks
@vinceau
Copy link
Member

vinceau commented Nov 19, 2023

Thanks for your help in looking into adding a database! We've finally integrated it into the repo in #400. :)

@vinceau vinceau closed this Nov 19, 2023
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