[WIP] Web API: Storage

[aduh95]

This PR implements window.sessionStorage with tests. The code has been successfully formatted, linted, and tested.

[hayd]

Should the scope of session storage be domain+protocol of the caller?

[kitsonk]

If I missed a conversation somewhere, but this doesn't really seem like an API we would want to implement in Deno. Providing this seems like very much something that could and should always exist in userland. The concept of "session" makes no sense for Deno.

What is the use case for this?

[aduh95]

What is the use case for this?

Fair question. I came to that while I was trying to adapt my library so it would work on deno, and I realize I could get rid of the calls to fs.writeFile I was making to store info on temp files if the Storage API was available. Besides, it's a step towards browser compatibility Deno is aiming for.

Should the scope of session storage be domain+protocol of the caller?

I interpreted "session" as "current process", which is arbitrary and questionable, although I don't think it should be related to the domain either IMHO; the specs states sessionStorage is specific to the current top-level browsing context, which means as MDN page says, "opening a page in a new tab or window will cause a new session to be initiated with the value of the top-level browsing context".

[ry]

@aduh95 This seems like a great candidate for an external module. If you can put this in your own repo, and submit a PR to add it to: https://github.com/denoland/registry/blob/master/x/database.json
Then your module could be available for include using

import { Storage } from "https://deno.land/x/storage/storage.ts";

[hayd]

Web compatibility is the reason to consider including session storage, e.g. it allows a "secret store" only available to scripts within domain+protocol. As an external/deno module this wouldn't really do anything besides wrap Map, to support session storage it needs to be baked in to deno. (Similarly for local storage.)

Are there examples where this is useful in external (web compatible) scripts?

[ry]

@hayd Ok - good point....

[aduh95]

@hayd that would be localStorage, and I agree that would be a big deal. Unfortunately, I was too lazy to implement that on this PR...

[aduh95]

I am going to take another look at it this week to try to come up with a more complete implementation that includes localStorage if we agree that's something we want on deno.

[hayd]

that would be localStorage

@aduh95 Isn't session storage "per-origin-per-window-or-tab" i.e. domain+protocol ?
(Perhaps I misunderstand your comment.)

[kitsonk]

@hayd I don't see any way that you would be able to implement what you are suggesting without making breaking the API. In the browser it is "per-origin-per-window-or-tab". A session in Deno is an instance of a running program. If you are suggesting something that stores information for an HTTP server, like session information, that is clearly higher order functionality of some sort of HTTP server, which we already agreed was something people should build on top of Deno.

This PR certainly isn't that, and in order to implement it, like I said, the only way to implement it would be to break the API. I don't even see why it would be required to be "baked into Deno". Code ensured that one client only wrote a particular in memory Map doesn't require any specific API's from Deno. It simply requires the user land author to do so.

[hayd]

I'm not talking about per request, sorry for the confusion.

A session in Deno is an instance of a running program

Agreed. So session storage for deno is a Map shared between scripts within domain+protocol. This would require sessionStorage know the domain+protocol of the caller.

[kitsonk]

domain+protocol of the caller

What caller? A client of a Deno HTTP server? The instance of an HTTP server listening on a port? Again, it is up to the programme to know what semantics to apply to those. That is like having a built in router in Deno for an HTTP Server and that is a higher order concern of something built upon Deno, not something integrated into Deno. The client and protocol are part of the request object, and Deno arbitrarily enforcing what can or cannot be executed in JavaScript and managing instances of Maps in JavaScript land seems crazy.

In any case, that isn't even close to this PR (not trying to take away from @aduh95 trying to contribute) and doesn't even begin to express what sort of permission segregation you are suggesting. If there is a good use case for something like that, it would be better to open an issue, so the approach can be discussed versus trying to make this into something it clearly isn't. I agree with @ry original assessment, this is a great API to have as an external module registered with deno.land.

[hayd]

this is a great API to have as an external module registered with deno.land.

I disagree, atm it's only a wrapper for Map.

The "caller" is the script file that's calling sessionStorage.getItem et al, if it's https://example.com/foo.ts then the domain/protocol is example.com/https.

[axetroy]

Because deno is compatible with the browser, I don't think it should be placed in a third-party package.

For example, if sessionStorage have been defined built in window object, you can use it like this.

sessionStorage.getItem()

But if put it in an external package, we have to use like this

import { sessionStorage } from "https://deno.land/x/storage/storage.ts";
sessionStorage.getItem()

This is not browser-compatible design.

Especially some libraries need run in server-side-rending which include the code: sessionStorage.getItem then deno will throw an error

[hayd]

Also, I think I was incorrect in my previous claim: sessionStorage should be global shared Map (and ignore domain), just like in this PR.

[aduh95]

I have started working on the localStorage implementation, and I have encountered a problem: Object.defineProperty seems not to be kept during the compilation step.

https://github.com/aduh95/deno/blob/86d734c3ea96564d942f8bba4518d3662d7ef1dc/js/globals.ts#L62-L71

I use this to lazy load the localStorage object as well as to avoid object being replaced in userland.

Am I doing something wrong there or is it something we need to fix in the compilation process?

[hayd]

Maybe for browser compatibility it'd be sufficient for local storage to work the same as session storage.

[ry]

Closing old PRs. Let's move this discussion to an issue or new PR - I haven't thought about it much TBH.