feat: add support for browser.storage.local backend - #162
Conversation
jupenur
left a comment
There was a problem hiding this comment.
I'm wondering if we could just have browser.storage.local as the only storage backend? Is IndexedDB preferable for some reason?
| await browser.storage.local.remove(oldKeys); | ||
| } | ||
|
|
||
| // Store new entries in chunks to avoid exceeding message size limits |
There was a problem hiding this comment.
What's the limit mentioned here? I can't find anything on MDN.
| const all = await browser.storage.local.get(null); | ||
| const oldKeys = Object.keys(all).filter((k) => | ||
| k.startsWith(`${this.prefix}list_`), | ||
| ); |
There was a problem hiding this comment.
This seems a bit inefficient. Maybe we could maintain an index under another key instead and do something like
const indexKey = `${this.prefix}index`;
const oldKeys = (await browser.storage.local.get(indexKey))[indexKey];| return new IndexedDbStorageBackend(db); | ||
| } | ||
|
|
||
| /** Try to use browser.storage.local and verify it is writable. */ |
There was a problem hiding this comment.
Is there any scenario where the browser.storage.local API would exist but not be writable? The only one I can think of is if we hit the storage quota, but I'm not sure falling back to in-memory storage is the right solution.
My original choice was for speed of writes and reads, especially as # of entries increase. I tried to find again supporting documentation for that but I couldn't. Ideally I agree it would be best to drop this abstraction and just use an API that is always available. I've run a quick generated benchmark script and this is the result (using clear() and writing everything in a single transaction for both):
Seems like list import is the only significant slowdown, though negligible at our current numbers. Instead of the manual key removal that as you say is inefficient, we could also call clear() because I don't think we are storing anything else. You are right and there's no per-transaction/per-message limit, it's an hallucination as it works in a single call even with 100MBs of data. I wonder what you think in light of the potential performance difference? I'm open to use only |
|
Are the numbers for the current logic where the In any case, I don't feel like the numbers are that bad for the scale we expect for now. Simplifying would probably be the greater benefit. |
The insert there was a single put with all the values, no chunking, but awaiting that. Anyway I agree, I'm gonna refactor with just that for now! |
|
I've dropped all the backends in favor of The other update information useful for scheduling update is migrated to |

As suggested in #98 (comment) this PR adds support to
browser.storage.localas a memory backend. The original class already supported testing IndexedDb and falling back on memory, while with this the order will be IndexedDb->storage.local->memory. It have tested in TBB and it seems to work.Integration tests are passing. I think it's still useful to keep the memory fallback for now, but we might evaluate if to remove it in the future.