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: sessionStorage #4973

Closed
wants to merge 1 commit into from
Closed

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Apr 28, 2020

Implements window.sessionStorage for Deno.

This is mostly a wrapper around Map, that should be 100% compatible with the browser implementation.

localStorage is a bit tricky to achieve on Deno, as far as I know their is no obvious equivalent to an origin on Deno, I have chosen to make it throw a SecurityError as the specs allows:

If the request violates a policy decision (e.g. if the user agent is configured to not allow the page to persist data), the user agent may throw a "SecurityError" DOMException instead of returning a Storage object

Missing features: window.localStorage, StorageEvent class and storage event.

Refs: https://html.spec.whatwg.org/multipage/webstorage.html#storage-2
Refs: #1657

@nayeemrmn
Copy link
Collaborator

nayeemrmn commented Apr 28, 2020

-1 if this isn't actually persisting anything... certainly #1657 shouldn't be closed, and this is going to cause confusion as to whether or not this is properly implemented.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 28, 2020

Yes, I say we need something that persists, even session storage. Until that point, any sort of "in memory" compatibility should be in deno.land/x.

Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

Again, I don't think we should consider something that doesn't actually persist... maybe session storage can be in memory, but if all we have is in memory, we shouldn't add it at the moment, and keep something like this in deno.land/x/

We really need to work out the semantics of the origin for the storage... a lot of those are discussed in #1657 but not addressed here yet.

@@ -234,6 +235,13 @@ export const windowOrWorkerGlobalScopeProperties = {
Worker: nonEnumerable(workers.WorkerImpl),
};

export const windowGlobalScopeProperties = {
Copy link
Contributor

Choose a reason for hiding this comment

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

While technically, web workers don't have access to local storage, I would not want to add another bag here. We already have things in worker global that shouldn't technically be there. @bartlomieju opinions?

Copy link
Member

Choose a reason for hiding this comment

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

I agree with you @kitsonk - workers shouldn't have access to localStorage

@aduh95 aduh95 changed the title Web API: Storage Web API: sessionStorage Apr 29, 2020
@aduh95
Copy link
Contributor Author

aduh95 commented Apr 29, 2020

I have updated the OP saying this PR doesn't actually implements localStorage. I think we should not disregard this PR just yet; although it's true it doesn't address the points discussed in #1657, my hope is that it can make the ball rolling for an actual implementation of persistant database in Deno.

@aduh95 aduh95 force-pushed the web-api-storage branch from b356334 to 56c9f00 Compare May 2, 2020 15:42
@bartlomieju bartlomieju added cli related to cli/ dir web related to Web APIs labels May 15, 2020
@bartlomieju
Copy link
Member

Thanks for opening the PR @aduh95 and sorry for slow review.

I agree with @nayeemrmn and @kitsonk opinions - this module should start as a 3rd party module in deno.land/x/ and make its way into public API after we figure out origin aspects. Closing without merge.

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.

5 participants