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

feat(ext): Unsafe sqlite #15

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat(ext): Unsafe sqlite #15

wants to merge 4 commits into from

Conversation

load1n9
Copy link
Member

@load1n9 load1n9 commented Aug 2, 2024

No description provided.

@load1n9 load1n9 requested a review from marc2332 August 2, 2024 05:41
cli/Cargo.toml Show resolved Hide resolved
cli/Cargo.toml Show resolved Hide resolved
Comment on lines +23 to +25
loader.init_storage(SQliteExtResources {
connection: Connection::open(":memory:").unwrap(),
});
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't seem like a good idea to open a new sqlite connection at startup, it should be manually triggered by the user by calling an op, just like internal_sqlite_execute

Copy link
Member

Choose a reason for hiding this comment

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

I now see it might be used for backing a localStorage-like API, in that case, how big does embedding sqlite make the binary, and I also wonder what's the performance hit for startup. Perhaps we could lazy load the connection whenever internal_sqlite_execute is executed, I think that would be the best thing to avoid a performance hit (if there is any).

Copy link
Member Author

Choose a reason for hiding this comment

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

i might do that when we can pass through pointers or external objects

Copy link
Member

Choose a reason for hiding this comment

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

You mean resources? We can already do that

Copy link
Member

Choose a reason for hiding this comment

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

Is Storage supposed to be backed by the sqlite? It seems to be missing an actual implementation right?

@@ -1,7 +1,11 @@
mod console;
mod fs;
#[cfg(feature = "unsafe-sqlite")]
Copy link
Member

Choose a reason for hiding this comment

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

question: Why is it called unsafe-sqlite instead of just sqlite? Just curious

Copy link
Member Author

Choose a reason for hiding this comment

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

because its super unsafe atm and should only be used when testing new features that may depend on sqlite

@marc2332
Copy link
Member

marc2332 commented Aug 4, 2024

You got a few conflicts @load1n9

@load1n9
Copy link
Member Author

load1n9 commented Aug 4, 2024

You got a few conflicts @load1n9

yeah lets not use this pr, we can use it as reference for anything needing sqlite

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