Storage API #1483
Storage API #1483
Conversation
| const binding = overrideStorage ? | ||
| new ViewerStorageBinding(window) : | ||
| new LocalStorageBinding(window); | ||
| return new Storage(window, viewer, binding).start_(); |
There was a problem hiding this comment.
Fixed. Didn't actually test this code yet :)
dvoytenko
force-pushed
the
storage1
branch
2 times, most recently
from
9db7622
to
590d727
Compare
|
@ericfs requesting the formal review. |
| } | ||
|
|
||
| /** | ||
| * Saves the specified property. Returns the promise that's resolved when |
dvoytenko
force-pushed
the
storage1
branch
2 times, most recently
from
7c5f5cc
to
0e5ed25
Compare
| saveStore(origin, store) { | ||
| return new Promise(resolve => { | ||
| this.win.localStorage.setItem(this.getKey_(origin), store); | ||
| resolve(); |
There was a problem hiding this comment.
This gives me automatic rejection if setItem method fails without needing to use try/catch directly.
|
LGTM |
|
I did some additional research today and need a little time to process it. I will hold off merging this PR in the meantime. @ericfs please hold off viewer work on this for a little bit as well. |
|
Ok, let me know if you're updating the PR. |
| if (this.store_) { | ||
| return Promise.resolve(this.store_); | ||
| } | ||
| return this.whenStarted_.then(() => { |
There was a problem hiding this comment.
Why not save this as a storePromise to avoid the extra allocations above?
|
Assigning to @cramforce for additional review. |
| Promise.resolve() : | ||
| Promise.reject(`Enable experiment ${EXPERIMENT}`); | ||
|
|
||
| /** @private {!Promise<!JSONObject>} */ |
There was a problem hiding this comment.
Can you use ! with a null promise?
There was a problem hiding this comment.
Nope. Fixed. (Where are you closure compiler?)
|
@cramforce PTAL. The format of the storage changed considerably to allow limits. In general, though, it looks like a good idea. |
| this.maxValues_ = opt_maxValues || MAX_VALUES_PER_ORIGIN; | ||
|
|
||
| /** @private @const {!Array<!JSONObject>} */ | ||
| this.values_ = obj['vv'] || []; |
There was a problem hiding this comment.
Why not use an Object<string, JSONObject>? That'll avoid the O(n) lookup times.
There was a problem hiding this comment.
Possible of course. The set is very small - forced to be under 8 elements. It simplifies search and removal of old values. And it also seemed a bit better as a format. But I can definitely switch to a map if you feel that format would improve.
There was a problem hiding this comment.
Oh, I see why you're doing it now (purging old values). Different comments incoming. 😉
|
Switched the new version to use hash format. |
|
@cramforce PTAL. |
| * @override | ||
| */ | ||
| set(name, value) { | ||
| assert(typeof value == 'boolean', 'Only boolean values accepted'); |
There was a problem hiding this comment.
We need to make sure to not provide a "read all keys" API. That could be used to put values into keys :)
|
LGTM |
|
@ericfs this is merged. I don't think viewer bindings have really changed at all. |
No description provided.