Implement Web Storage #1700

dsseng · 2019-02-07T10:23:17Z

Storage interface
sessionStorage
localStorage
Implement localStorage #1657.

kitsonk · 2019-02-07T23:07:10Z

These need to align to what is in lib.dom.d.ts. There is a single global interface Storage and constructor function (window.Storage) which needs to be newable. Also sessionStorage instanceof Storage === true.

The same goes for localStorage. You are going to have to manage whether you persist the data internally without change the API. I would recommend exporting Storage, sessionStorage, and localStorage from storage and not new them up in globals.ts but rather new them up inside the module. For the localStorage you would when persist the changes.

With the values (and possibly the keys) I think we need some sort of runtime checking (or coercion), because people will expect it if they are using just JavaScript. For example, this works without throwing an error:

sessionStorage.setItem("foo", 1);

It will return always return a string of course. So I would say that you would want to simply do a String(value) when setting a value.

@ry at the moment we don't have a concept of window.onunload within Deno, and we are going to need something like that so that localStorage can be serialised and persisted to the local file system.

aduh95 · 2019-02-12T21:33:41Z

sessionStorage is supposed to be readonly as per the specs. Should we try to have that in Deno as well?

dsseng · 2019-03-16T09:56:20Z

@ry maybe transfer this to deno_std?

zekth · 2019-03-16T10:48:09Z

js/storage.ts

+  key(index: number): string {
+    return Object.keys(this._storage)[index];
+  }
+  getItem(key: string): string {


Would it be revelant to return an empty string instead of an undefined value in this case?

@zekth I don't think so. Maybe user put "" there? undefined will let user see that it's... undefined!

Both solution are good, it's more like a philosophy. Some APIs returns empty strings, others undefined as the return type is a string. Was just wondering.

aduh95

This implementation is lacking support for property getter/setter/deleter (EG: window.sessionStorage['key']= 0 or delete window.sessionStorage['key']) as defined in the HTML specs. We need a Proxy to implement that.

Some other points also differ from the HTML spec, I have tried to list them in my review.

aduh95 · 2019-05-07T19:45:52Z

js/storage.ts

+    return Object.keys(this._storage)[index];
+  }
+  getItem(key: string): string {
+    return this._storage[key];


The spec says:

Returns the current value associated with the given key, or null if the given key does not exist in the list associated with the object.

Suggested change

return this._storage[key];

return this._storage[key] || null;

aduh95 · 2019-05-07T20:00:53Z

js/storage.ts

+  key(index: number): string {
+    return Object.keys(this._storage)[index];
+  }
+  getItem(key: string): string {


Suggested change

getItem(key: string): string {

getItem(key: string): string | null {

aduh95 · 2019-05-07T20:06:53Z

js/session_storage_test.ts

+  assertEqual(sessionStorage.key(0), "test");
+  assertEqual(sessionStorage.key(1), "test1");
+  sessionStorage.clear();
+  assertEqual(sessionStorage.key(0), undefined);


Suggested change

assertEqual(sessionStorage.key(0), undefined);

assertEqual(sessionStorage.key(0), null);

aduh95 · 2019-05-07T20:07:04Z

js/session_storage_test.ts

+  assertEqual(sessionStorage.getItem("test"), "John Doe");
+  assertEqual(sessionStorage.length, 1);
+  sessionStorage.clear();
+  assertEqual(sessionStorage.getItem("test"), undefined);


Suggested change

assertEqual(sessionStorage.getItem("test"), undefined);

assertEqual(sessionStorage.getItem("test"), null);

aduh95 · 2019-05-07T20:07:15Z

js/session_storage_test.ts

+  sessionStorage.setItem("test", "John Doe");
+  assertEqual(sessionStorage.getItem("test"), "John Doe");
+  sessionStorage.removeItem("test");
+  assertEqual(sessionStorage.getItem("test"), undefined);


Suggested change

assertEqual(sessionStorage.getItem("test"), undefined);

assertEqual(sessionStorage.getItem("test"), null);

aduh95 · 2019-05-07T20:07:22Z

js/session_storage_test.ts

+});
+
+test(function removeItems() {
+  assertEqual(sessionStorage.getItem("test"), undefined);


Suggested change

assertEqual(sessionStorage.getItem("test"), undefined);

assertEqual(sessionStorage.getItem("test"), null);

aduh95 · 2019-05-07T20:07:31Z

js/session_storage_test.ts

+import { test, assertEqual } from "./test_util.ts";
+
+test(function storeItems() {
+  assertEqual(sessionStorage.getItem("test"), undefined);


Suggested change

assertEqual(sessionStorage.getItem("test"), undefined);

assertEqual(sessionStorage.getItem("test"), null);

aduh95 · 2019-05-07T20:15:01Z

js/storage.ts

+  private _storage: Record<string, string> = {};
+
+  get length(): number {
+    return Object.keys(this._storage).length;


IMHO using a Map instead of an Object would make this way more efficient.

aduh95 · 2019-05-07T20:16:55Z

js/globals.ts

@@ -96,4 +97,8 @@ export type TextDecoder = textEncoding.TextDecoder;

 window.performance = new performanceUtil.Performance();

+export type Storage = storage.Storage;
+export type SessionStorage = storage.SessionStorage;
+window.sessionStorage = new storage.SessionStorage();


This should be a readonly property.

aduh95 · 2019-05-07T20:17:47Z

js/globals.ts

@@ -96,4 +97,8 @@ export type TextDecoder = textEncoding.TextDecoder;

 window.performance = new performanceUtil.Performance();

+export type Storage = storage.Storage;
+export type SessionStorage = storage.SessionStorage;


Do we need to export this type? It is not part of the HTML spec.

bartlomieju · 2019-05-20T09:53:50Z

@sh7dm will you update this PR?

ry · 2019-07-23T13:34:24Z

I'm still interested in adding built-in web storage support, but this needs more work and has now grown stale. Thanks @sh7dm for getting the ball rolling, I think we'll refer back to this in the future.

Implement Web Storage, sessionStorage

034d9ef

zekth reviewed Mar 16, 2019

View reviewed changes

aduh95 suggested changes May 7, 2019

View reviewed changes

ry closed this Jul 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Web Storage #1700

Implement Web Storage #1700

dsseng commented Feb 7, 2019 •

edited

Loading

kitsonk commented Feb 7, 2019

aduh95 commented Feb 12, 2019

dsseng commented Mar 16, 2019 •

edited

Loading

zekth Mar 16, 2019

dsseng Mar 16, 2019

zekth Mar 16, 2019

aduh95 left a comment •

edited

Loading

aduh95 May 7, 2019

aduh95 May 7, 2019

aduh95 May 7, 2019

aduh95 May 7, 2019

aduh95 May 7, 2019

aduh95 May 7, 2019

aduh95 May 7, 2019

aduh95 May 7, 2019

aduh95 May 7, 2019

aduh95 May 7, 2019

bartlomieju commented May 20, 2019

ry commented Jul 23, 2019

	return this._storage[key];
	return this._storage[key] \|\| null;

	getItem(key: string): string {
	getItem(key: string): string \| null {

	assertEqual(sessionStorage.key(0), undefined);
	assertEqual(sessionStorage.key(0), null);

	assertEqual(sessionStorage.getItem("test"), undefined);
	assertEqual(sessionStorage.getItem("test"), null);

Implement Web Storage #1700

Implement Web Storage #1700

Conversation

dsseng commented Feb 7, 2019 • edited Loading

kitsonk commented Feb 7, 2019

aduh95 commented Feb 12, 2019

dsseng commented Mar 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduh95 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartlomieju commented May 20, 2019

ry commented Jul 23, 2019

dsseng commented Feb 7, 2019 •

edited

Loading

dsseng commented Mar 16, 2019 •

edited

Loading

aduh95 left a comment •

edited

Loading