-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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(extensions): Add WebStorage API #7819
Conversation
# Conflicts: # cli/rt/99_main.js # cli/worker.rs
Closes #1657 |
# Conflicts: # cli/rt/99_main.js # cli/worker.rs
# Conflicts: # Cargo.lock # cli/Cargo.toml # cli/ops/mod.rs # runtime/js/14_webstorage.js # runtime/worker.rs
# Conflicts: # cli/standalone.rs
Only missing things are typings & tests |
# Conflicts: # Cargo.lock # runtime/Cargo.toml # runtime/build.rs # runtime/js/99_main.js
I'm not sure about using sqlite for localStorage. Sqlite like quite a lot of code for a simple key/value store. I could imagine a much smaller backend store where we simply use a directory of files, one for each KV pair, and a file system lock to manage concurrent access. Probably this could be done in 50 lines of Rust compared to the 100k of C that is Sqlite. There are also other solutions: leveldb, rocksdb, sled. We need some analysis of the tradeoffs between these various local databases and our future use-cases for them (particularly for IndexDB). I'd love to get this feature in - so I suggest this very simple (very suboptimal) directory of files storage backend - meanwhile we can figure out the long term database needs for Deno. Or we can do this analysis now... |
I have already looked at leveldb, rocksdb, & sled (and others). concurrent access with those is not possible, so those are already not an option. The reason for SQLite is because IndexedDB. Firefox uses SQLite for IndexedDB. Using SQLite was the decision i came up with after looking at various possible DBs and what we can use use for both Web Storage and IndexedDB. I dont mind changing, till it covers the needed requirements |
Parallel process access is certainly possible - it just isn't handled by the database itself. Those libraries have decided to keep it simple and leave the FS locking to the caller; that doesn't mean they are inherently unusable. |
@ry i just tried rocksdb, leveldb & sled for the implementation. sled is not an option. just running WPT on it causes it to fire rocksdb is somewhat painful to use, and the crate doesnt implement all functions. also building causes an invalid memory reference error, so i couldnt test it. leveldb is better to use than rocksdb, but the crate is not actively maintained (last commit 7 months ago), and doesnt include all functions, making a few things complicated. |
# Conflicts: # Cargo.lock # runtime/Cargo.toml # runtime/build.rs # runtime/js/99_main.js # runtime/ops/mod.rs # runtime/worker.rs
# Conflicts: # extensions/webstorage/01_webstorage.js # extensions/webstorage/Cargo.toml # extensions/webstorage/README.md # extensions/webstorage/lib.deno_webstorage.d.ts # extensions/webstorage/lib.rs # runtime/Cargo.toml
I think I messed up the merge commit somehow and am getting an error like this
I assume there's a simple fix, but I'm not able to immediately find it. Otherwise LGTM. Very nice work @crowlKats this is a great feature to have. Thank you very much for putting up with the endless reviews and questioning of backends. |
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 a lot for the work on this @crowlKats. Great feature to have :-)
Closes #1657
This PR does not implement the storage events.