Skip to content

Update codebase #154

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

Merged
merged 10 commits into from
May 25, 2025
Merged

Update codebase #154

merged 10 commits into from
May 25, 2025

Conversation

sigmaSd
Copy link
Contributor

@sigmaSd sigmaSd commented May 23, 2025

whats missing is updating benchmarks, seems some are outdated like wasm sqlite

@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 23, 2025

Actually the ci I also needs some work, I updated it, but I'll need to test it first so I'll mark this as draft

@sigmaSd sigmaSd marked this pull request as draft May 23, 2025 09:03
@sigmaSd sigmaSd changed the title Update database Update codebase May 23, 2025
@sigmaSd sigmaSd marked this pull request as ready for review May 23, 2025 13:33
@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 23, 2025

I tested and fixed the ci it should work now, also I noticed that the libraries are not stripped, I think they should be for example the linux x86_64 one goes from 8.9mb to 1.mb

If you agree I can also add that to the ci

@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 23, 2025

Actually I just remembered that now there is node:sqlite support https://docs.deno.com/api/node/sqlite/, maybe that's why development slowed down a bit

@DjDeveloperr
Copy link
Member

Hi - sorry the development slowed down a bit as I got busy with other projects and work. I'll get back to this PR soon as I find some time. I had looked into node:sqlite, and the performance isn't on par with the FFI library afaik. Will have to run benchmarks to be sure.

@DjDeveloperr
Copy link
Member

I agree with stripping the libraries, we should do that. For now this PR is good, I'll merge it so we have the bare minimum updates on main for now

Copy link
Member

@DjDeveloperr DjDeveloperr left a 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, I really appreciate it

@DjDeveloperr DjDeveloperr merged commit 71e42af into denodrivers:main May 25, 2025
@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 25, 2025

No problem thanks as well , its a nice library and the bench idea is really cool

@sigmaSd
Copy link
Contributor Author

sigmaSd commented May 25, 2025

I updated just before you merge its just missing a deno fmt run to fix the ci issue

@DjDeveloperr
Copy link
Member

Oh I just realized haha, thought I was tripping. Thanks for the update, will format it

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