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

Add database session and fix store session #4

Merged
merged 11 commits into from Jul 17, 2022
Merged

Add database session and fix store session #4

merged 11 commits into from Jul 17, 2022

Conversation

watzon
Copy link
Contributor

@watzon watzon commented Jul 15, 2022

This PR adds a DatabaseSession which uses denodb to allow session creation with Postgres, MySQL, MariaDB, SQLite, and MongoDB. For now this makes us dependent on denodb, which I don't love, but that could be mitigated by separating this out into its own package or relying on some kind of dependency injection.

I also managed to fix StoreSession. It was actually just a single missed line that was keeping it from working.

@dcdunkan
Copy link
Member

This is amazing! Nice work. I'll do a review within a few hours. Thank you so much.

I also managed to fix StoreSession. It was actually just a single missed line that was keeping it from working.

Haha. Thanks for that too! If I remember correctly. I rewrote those session files, rather than copying it. And I never tested or re-checked it. Thank you for the fix <3

Copy link
Member

@dcdunkan dcdunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I'll merge as soon as the discussions are resolved.

I was thinking about adding more examples on using grm. I'd appreciate if you can add a minimal example on using this database session.

deps.ts Show resolved Hide resolved
src/sessions/database_session.ts Outdated Show resolved Hide resolved
@watzon
Copy link
Contributor Author

watzon commented Jul 16, 2022

There is one issue that I'm having, and I need someone else to test and see if this is just me or a bigger underlying problem. Some channels seem to be failing to get added to the entities, but also not. For instance my bot's log channel (which is private).

This problem only happens on restart. If you send a message in the channel while the bot is running, and then look for that channel with the bot you'll find it, but once the bot restarts it's gone again.

@watzon
Copy link
Contributor Author

watzon commented Jul 16, 2022

Never mind, I fixed the persistence issues. Still not 100% sure why it's fixed, but everything works as expected now.

Copy link
Member

@dcdunkan dcdunkan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with the file names, rename the databaseSession.ts file to database_session.ts.

And also, there is a conflict as you may have noticed. So it's the examples/basic.ts file now. Not simple.ts.

examples/databaseSession.ts Outdated Show resolved Hide resolved
examples/databaseSession.ts Outdated Show resolved Hide resolved
examples/databaseSession.ts Outdated Show resolved Hide resolved
@watzon
Copy link
Contributor Author

watzon commented Jul 16, 2022

I believe we're good now

@dcdunkan
Copy link
Member

dcdunkan commented Jul 16, 2022

I think no. We have issues with examples as I see. Seems like you now only have databaseSession.ts. We need both basic and databaseSession examples. What I meant about renaming was to rename databaseSession.ts to database_session.ts. And not touch basic.ts.

@dcdunkan
Copy link
Member

dcdunkan commented Jul 16, 2022

If that's done, I think we're good and I'll merge this

@dcdunkan
Copy link
Member

Its okay if you can't figure out the issue with the examples. Just revert adding the examples. I'll add them myself later.

@watzon
Copy link
Contributor Author

watzon commented Jul 16, 2022

Wait, did it get removed? My branch has it locally and shows that the changes were pushed. I thought it wasn't showing up just because that file now matches the main branch, so it's not showing changes.

@dcdunkan
Copy link
Member

@watzon It looks like it is removed/modified as database_session.ts. Can you fix that, and resolve these conflicts? We had some breaking changes in #6

image

@watzon
Copy link
Contributor Author

watzon commented Jul 17, 2022

Man rebasing is not my strong suit, but I think I got everything in place

@dcdunkan
Copy link
Member

Is everything working alright? I haven't tested this stuff yet. If everything's fine, I'll merge this.

@watzon
Copy link
Contributor Author

watzon commented Jul 17, 2022 via email

@dcdunkan
Copy link
Member

I think I'll merge it. We're on 0.x anyway. Thanks for the PR!

@dcdunkan dcdunkan merged commit 7d476b6 into grmjs:main Jul 17, 2022
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.

None yet

2 participants