-
Notifications
You must be signed in to change notification settings - Fork 225
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
[kvdb] add an implementation for a browser #194
Conversation
@@ -0,0 +1,25 @@ | |||
[package] | |||
name = "kvdb-web" | |||
version = "0.0.1" |
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.
Should be at least 0.1.0, so that we can publish patches.
version = "0.0.1" | |
version = "0.1.0" |
@@ -0,0 +1,15 @@ | |||
[package] | |||
name = "kvdb_web_demo" | |||
version = "0.0.1" |
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.
version = "0.0.1" | |
version = "0.1.0" |
description = "Playground for kvdb in the browser" | ||
license = "GPL-3.0" | ||
edition = "2018" | ||
|
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.
Let's add this, to prevent accidentally publishing the demo:
publish = false |
type Key = BytesHexEncoding; | ||
type Value = BytesHexEncoding; | ||
|
||
macro_rules! console_log { |
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.
We should just use log!
and warn!
from the log
crate, and the user can redirect them to the console, for example using https://crates.io/crates/console_log
[dependencies.web-sys] | ||
version = "0.3" | ||
features = [ | ||
'console', |
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.
Can remove console
; see my remarks about logging.
} | ||
|
||
fn flush(&self) -> io::Result<()> { | ||
Ok(()) |
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.
Shouldn't that call do_persist_db
?
.expect_throw("localStorage should be supported in your browser") | ||
.unwrap_throw(); | ||
|
||
let a = Closure::wrap(Box::new(move || do_persist_db(&db.clone(), &local_storage)) as Box<dyn Fn()>); |
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.
We should use a Weak<Database>
here, so that if we close the database, the flushing stops as well.
Ideally we would also stop the timer. This can be done by using set_timeout
instead of set_interval
, and having the callback call set_timeout
again, unless the database is dead.
I'm not sure of the difficulty of doing that, and I'd be fine with merging with set_interval
.
closing in favor of #202 |
This is a simple implementation on top of localStorage, which serializes the whole DB every 2 seconds. Hopefully good enough for a demo.
cc #182, cc @tomaka