-
-
Notifications
You must be signed in to change notification settings - Fork 657
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
feat(database): add support for BunSQLite and refactor related code #2944
Conversation
I havnt updated the documentation yet, Just want to discuss which category or section I should put it in |
src/module.ts
Outdated
if (!process.versions.bun && options.database.type === 'bunsqlite') { | ||
logger.warn('BunSQLite is not available in this environment. Falling back to SQLite.') | ||
options.database = { | ||
type: 'sqlite', | ||
filename: options.database.filename, | ||
} | ||
} |
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.
This is a duplicate of previous if
Lets change it and check for using sqlite
in bun environment
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.
Im checking both the _localDatabase aswell as the database key in options. Dont we want to check both?
What we could do is check if the database type is sqlite and then just use the new method getDefaultSqliteAdapter and if there was a mismatch log a warning
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.
Thank you for the PR
There is an issue that we should fix. running built version with Bun fails because Bun does not support CompressionStream
internally.
As for documentation, we can mention bunsqlite
alongside other database adapters https://content3.nuxt.dev/docs/getting-started/configuration#database
I was wondering what if, instead of creating a separate adapter for This way it would be less confusing and Content will have one |
Added suggested Polyfill Co-authored-by: Farnabaz <farnabaz@gmail.com>
That would work aswell, I just made it into 2 adapters instead since they will have seperate calls. Bun has taken some stuff from better-sqlite3 when it comes to their methods and overall structure but there are differences. Just thought it better to split the code into 2 different files. We could remove the options for bun for both _localDatabase and database and just default it to the environment one. |
@farnabaz I have looked over the things you said and I think I have covered it all. |
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.
LGTM
Thanks β€οΈ
@farnabaz Do you have an estimate on when this will be released? |
@felixrydberg alpha.9 is released. https://github.com/nuxt/content/releases/tag/v3.0.0-alpha.9 |
@farnabaz I have found a very specific issue with our Polyfill. I have created a reproduction repo here: https://github.com/felixrydberg/content-bun-error It works fine when running the output file, but when building it into a single bun executable the polyfill will get an unexpected end of file from node:zlib |
π Linked issue
#2936
β Type of change
π Description
Resolves #2936
Added bun:sqlite as an alternative to better-sqlite-3, both for database and _localDatabase config options.
Refactored localDatabase function that used better-sqlite-3 to be extendable with more options in the future.
π Checklist