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

Web API: window.localStorage #5052

Closed
wants to merge 3 commits into from

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented May 2, 2020

Implementation of window.localStorage on top of #4973 using the sled crates. Please ignore the sessionStorage implementation, this PR focuses on commit b3c225f.

This should not land as is, I built it as a proof of concept of what a localStorage implementation for Deno could look like. We would need to discuss about:

Regarding the implementation, I tried to stick to the HTML specs and failed:

Blocked by #4973

aduh95 added 3 commits May 2, 2020 17:38
Implements window.localStorage using sled. Work in progress.
@aduh95 aduh95 force-pushed the localStorage-implementation branch from f71af62 to 1b124b2 Compare May 2, 2020 16:33
export const windowGlobalScopeProperties = {
localStorage: getterOnly(() => {
// @ts-ignore
const { origin } = location;
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'd say there's no way to land anything localStorage related without reintroducing globalThis.location and first deciding what it means.

@bartlomieju bartlomieju added cli related to cli/ dir web related to Web APIs labels May 15, 2020
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

Thanks for opening the PR and the effort @aduh95, however similarly to #4973 before introducing storages that depend on origin we need to figure out a Web compatible way for deciding on location.

It'd be better to first spec out such complex feature in a design doc and get some consensus before implementing it (especially if it pulls new Rust crates).

Closing without merge.

@@ -50,6 +50,7 @@ rustyline = "6.1.0"
serde = { version = "1.0.106", features = ["derive"] }
serde_derive = "1.0.106"
serde_json = { version = "1.0.51", features = [ "preserve_order" ] }
sled = "0.31.0"
Copy link
Member

Choose a reason for hiding this comment

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

sled is still considered unstable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli related to cli/ dir web related to Web APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants