Added SQLite event store the ability to create connection #178
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Moved creation of db by location to construction
davecosec
changed the title
oskardudycz
changed the title
oskardudycz
reviewed
Jan 25, 2025
| autoMigration?: 'None' | 'CreateOrUpdate'; | ||
| }; | ||
| }; | ||
| export type SQLiteEventStoreOptions = |
There was a problem hiding this comment.
SUGGESTION: @davecosec can we also allow passing sqlite3.Database? I think that will be useful to do without passing the sqliteConnection. I know that in the future we'll allow more providers, but for now we have only one, so I think that this would be accessible to add.
oskardudycz
reviewed
Jan 25, 2025
oskardudycz
reviewed
Jan 25, 2025
oskardudycz
requested changes
Jan 25, 2025
There was a problem hiding this comment.
@davecosec, that's good work. I added some small requests to adjust the database injection and lifetime management. Once that's made, I'm happy to pull that in.
Feel free to ask if I wasn't precise in the comments. 🙂
oskardudycz
force-pushed
the
main
branch
2 times, most recently
from
697ae3d to
3eda928
Compare
oskardudycz
reviewed
Feb 8, 2025
oskardudycz
reviewed
Feb 8, 2025
| } | ||
|
|
||
| if (options?.databaseLocation == null) { | ||
| throw new Error('Database location is not set'); |
There was a problem hiding this comment.
CONCERN: Wouldn't it be better to make database location required? Then, on the compiler level, we would make it impossible to provide the wrong value. The alternative is to keep it optional and use in-memory if not provided, which I think is fine.
|
@davecosec could you check why this test is faling in CI? |
oskardudycz
reviewed
Feb 8, 2025
oskardudycz
force-pushed
the
sqlite-updated-event-store-to-allow-for-connection-creation
branch
3 times, most recently
from
acf1d20 to
a974b8e
Compare
oskardudycz
approved these changes
Feb 11, 2025
E2E tests were failing, as there was no nested database folder in the 'testing' subdirectory. Probably locally it was created, but git is not commiting empty folders. Adjusted not to require such nesting. Fixed also TS config to compile SQLite package correctly, as some entries were missing. After that fixed the common errors. Added a helper withConnection to streamline the connection management. Now it also uses correctly close in finalize, instead of double closing in case of error. Renamed location to fileName to follow the SQLite naming convention, made it also optional with fallback to in memory. Removed absolute file path and custom, as it won't allow easily passing the filenames without casting that are not typed manually.
oskardudycz
force-pushed
the
sqlite-updated-event-store-to-allow-for-connection-creation
branch
from
a974b8e to
913f5e6
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Moved creation of db by location to construction