catch failed setItem #3660
catch failed setItem #3660
Conversation
| return new Promise(resolve => { | ||
| return new Promise((resolve, reject) => { | ||
| if (!this.win.localStorage) { | ||
| reject('No localStorage object found'); |
There was a problem hiding this comment.
I'm not sure these two rejects are needed. If an error is thrown inside the resolver (and resolve has not been called yet), the promise rejects with that error.
There was a problem hiding this comment.
yeah these do get caught already its just the type of error i wanted to clarify. (from logging pov). I probably won't merge this anyways and just filter them from our logs to a new metric
erwinmombay
force-pushed
the
localstorage-get-set
branch
2 times, most recently
from
7f2e189
to
54e45cb
Compare
erwinmombay
changed the title
|
LGTM. |
| @@ -135,6 +135,9 @@ export class Storage { | |||
| const blob = btoa(JSON.stringify(store.obj)); | |||
| return this.binding_.saveBlob(this.origin_, blob); | |||
| }) | |||
| .catch(reason => { | |||
There was a problem hiding this comment.
Let's update on the binding level.
There was a problem hiding this comment.
i believe this is obsolete.
erwinmombay
force-pushed
the
localstorage-get-set
branch
from
54e45cb
to
b03bd5b
Compare
|
@dvoytenko PTAL. not sure if we still want to report localStorage support. |
|
@erwinmombay We still do want to report it. Our goal is to ensure it's not growing. If it would - that'd likely indicate a new client that has storage misconfigured. |
| @@ -302,13 +305,21 @@ export class LocalStorageBinding { | |||
| /** @override */ | |||
| loadBlob(origin) { | |||
| return new Promise(resolve => { | |||
| if (!this.isLocalStorageSupported_) { | |||
| resolve(); | |||
There was a problem hiding this comment.
Since loadBlob is defined as a promise resolving null or string - let's do resolve(null);
erwinmombay
force-pushed
the
localstorage-get-set
branch
from
b03bd5b
to
425ff0b
Compare
|
LGTM pending test for |
erwinmombay
force-pushed
the
localstorage-get-set
branch
from
425ff0b
to
ba269cc
Compare
No description provided.