Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improved Storage API (looking for feedback) #228

Closed
aklinker1 opened this issue Nov 11, 2023 · 19 comments · Fixed by #234 or #300
Closed

Improved Storage API (looking for feedback) #228

aklinker1 opened this issue Nov 11, 2023 · 19 comments · Fixed by #234 or #300
Assignees
Labels
Milestone

Comments

@aklinker1
Copy link
Collaborator

aklinker1 commented Nov 11, 2023

Feature Request

The storage APIs are very hard to use, so WXT should provide a standard storage API that's much simpler to use.

Here's an idea for the implementation using unstorage:
import { WatchCallback, createStorage, defineDriver } from "unstorage";
import type { Storage } from "webextension-polyfill";

const webExtensionDriver = defineDriver<{
  storageArea: "sync" | "local" | "managed" | "session";
}>((opts) => {
  if (browser.storage == null)
    throw Error(
      "You must request the 'storage' permission to use webExtensionDriver"
    );

  // Only ever create a single listener per storage area
  const storageListener: (
    changes: Storage.StorageAreaSyncOnChangedChangesType
  ) => void = (changes) => {
    Object.entries(changes).forEach(([key, { newValue }]) => {
      _listeners.forEach((callback) => {
        callback(newValue ? "update" : "remove", key);
      });
    });
  };
  browser.storage[opts.storageArea].onChanged.addListener(storageListener);
  const _listeners = new Set<WatchCallback>();

  return {
    name: "web-extension:" + opts.storageArea,
    async hasItem(key) {
      const res = await browser.storage[opts.storageArea].get(key);
      return res[key] != null;
    },
    async getItem(key) {
      const res = await browser.storage[opts.storageArea].get(key);
      return res[key] ?? null;
    },
    async getItemRaw(key) {
      const res = await browser.storage[opts.storageArea].get(key);
      return res[key] ?? null;
    },
    async getItems(items) {
      const res = await browser.storage[opts.storageArea].get(
        items.map((item) => item.key)
      );
      return items.map((item) => ({
        key: item.key,
        value: res[item.key] ?? null,
      }));
    },
    async setItem(key, value) {
      await browser.storage[opts.storageArea].set({ [key]: value ?? null });
    },
    async setItemRaw(key, value) {
      await browser.storage[opts.storageArea].set({ [key]: value ?? null });
    },
    async setItems(items) {
      const map = items.reduce<Record<string, any>>((map, item) => {
        map[item.key] = item.value ?? null;
        return map;
      }, {});
      await browser.storage[opts.storageArea].set(map);
    },
    async removeItem(key) {
      await browser.storage[opts.storageArea].remove(key);
    },
    async getKeys() {
      const all = await browser.storage[opts.storageArea].get();
      return Object.keys(all);
    },
    async clear() {
      await browser.storage[opts.storageArea].clear();
    },
    watch(callback) {
      _listeners.add(callback);
      return () => void _listeners.delete(callback);
    },
  };
});

const storage = createStorage();
storage.mount("local", webExtensionDriver({ storageArea: "local" }));
storage.mount("session", webExtensionDriver({ storageArea: "session" }));
storage.mount("sync", webExtensionDriver({ storageArea: "sync" }));
storage.mount("managed", webExtensionDriver({ storageArea: "managed" }));
export default storage;

I might try and submit webExtensionDriver to unstorage in a PR. I know the storage listener setup isn't optimized, but it works for a basic setup. I wouldn't add the listener until its needed in WXT's implementation.

Usage would looks something like this:

const installDate = await storage.getItem("local:install-date");

storage.watch("session:counter", (event, key) => {
  console.log(`${key} ${event}d!`); // session:counter updated/removed!
});
await storage.setItem("session:counter", 123);

Is your feature request related to a bug?

Related to #227

What are the alternatives?

Here are some alternative storage libraries:

@aklinker1 aklinker1 changed the title Improved Storage API Improved Storage API (looking for feedback) Nov 11, 2023
@aklinker1 aklinker1 pinned this issue Nov 11, 2023
@aklinker1 aklinker1 added this to the v1.0 milestone Nov 11, 2023
@dvlden
Copy link
Contributor

dvlden commented Nov 13, 2023

I have nothing to add. I like the impl. idea and unstorage seems to be amazing.
The usage looks quite native, with the exception to watch instead of listen for example, but watch sounds better, indeed...

I'd simplify it even further:

const installDate = await storage.get("local:install-date");

storage.watch("session:counter", (event, key) => {
  console.log(`${key} ${event}d!`); // session:counter updated/removed!
});

await storage.set("session:counter", 123);

I do not think there's need for Item suffix. I'm looking forward to this feature!

@aklinker1 aklinker1 self-assigned this Nov 13, 2023
@aklinker1
Copy link
Collaborator Author

@dvlden If I'm using unstorage, I'll be using their Storage interface and won't remove the Item suffix from the method names.

@dvlden
Copy link
Contributor

dvlden commented Nov 13, 2023

Oh yea, makes sense. 🤣

@aklinker1 aklinker1 unpinned this issue Nov 15, 2023
@aklinker1
Copy link
Collaborator Author

Released in v0.10.0

@dvlden
Copy link
Contributor

dvlden commented Nov 16, 2023

After using impl. of unstorage, I find it weird that in comparison of built-in storage, watcher doesn't return value(s), but only key.

Is it possible to hijack the implementation so that it return k:v pair that changed?

@aklinker1
Copy link
Collaborator Author

It's definitely not intended, and I didn't explore that. I think I can append an argument to the watch callback without breaking unstorage's interface. I'll add that, because I agree, the vue composable I shared is a little inefficient without it.

@aklinker1
Copy link
Collaborator Author

aklinker1 commented Nov 16, 2023

I gave it a try, but it didn't work. Because we define a driver instead of the storage object itself, passing additional arguments into the callback are ignored inside unstorage.

So no, this is not possible if using unstorage. I'd consider moving to a fully custom storage util if unstorage isn't good enough, but not right now. I have other things I need to work on, and it's in a usable state.

Feel free to contribute a different implementation if this isn't good enough for you.

@aklinker1
Copy link
Collaborator Author

Or open a PR on unstorage to add the feature.

@aklinker1 aklinker1 reopened this Nov 16, 2023
@dvlden
Copy link
Contributor

dvlden commented Nov 16, 2023

I understand. Was hoping it's possible, but at the moment on the watcher I have to get the current value with the key, so I guess it's not that big of a deal, was just hoping to avoid that extra step.

No clues why they decided to go with this specific approach on the watcher, but I guess it's for a good reason.

I tried to use the snippet you wrote for me in Vue, previously with two dependencies:
#227 (comment)

  1. provide/inject
  2. useAsyncState

But I failed to make it work with new wxt/storage. I'll need to play around more.
At the moment, I'm using your unstorage driver as is, making a mess in my codebase... Testing if all puzzles fit, before I dig in deep into extension details.

What can I say man, you did some amazing work in such short time, while I've been testing wxt and I wish I knew this much to help with contributions, but some of the source code magic I simply don't understand.

@aklinker1
Copy link
Collaborator Author

But I failed to make it work with new wxt/storage. I'll need to play around more.
At the moment, I'm using your unstorage driver as is, making a mess in my codebase... Testing if all puzzles fit, before I dig in deep into extension details.

I'll share the entire example, maybe that will help.

@dvlden
Copy link
Contributor

dvlden commented Nov 16, 2023

If you get time for it...
I made a contribution, I guess this will help wxt in the future, if they approve it?

unjs/unstorage#342

Not sure I did it right, but looks good to me 👍

@aklinker1
Copy link
Collaborator Author

@dvlden Here's a full example with a working composable:

https://github.com/wxt-dev/wxt-examples/blob/main/examples/vue-storage-composable/README.md

@aklinker1
Copy link
Collaborator Author

aklinker1 commented Dec 24, 2023

Was using the storage API yesterday, and realized unstorage saves things as strings (at least it saves numbers as strings).

await storage.setItem("local:installDate", Date.now());
await storage.getItem("local:installDate"); // "1703427743358" instead of 1703427743358

So that's not acceptable... Will probably implement a custom storage API soon.

@aklinker1
Copy link
Collaborator Author

aklinker1 commented Dec 24, 2023

Here's what I'm thinking considering the feedback in this thread, my personal experience, and research into other libraries:

  1. I like unstorage's API and patterns
    • While getItem is verbose compared to get, it is fairly standard. It also allows for obvious plural forms of the functions, like getItems. It also allows for other setters, like setMeta, which could be useful in versioning fields.
    • The : separated keys are a nice approach, and using the local:/session:/sync:/managed: prefixes have worked well
  2. Change listener will report the new and old values
  3. I like the idea of creating a StorageItem from webext-storage, but recognize that it's not something everyone would use
    • I would prefer this over adding a storage schema to define the keys and their types, because that leads to circular imports
    • Migrations is always something I've wanted from a storage API, this could be a good
  4. I've never had performance issues with reading and writing directly to storage, so I don't see a reason to add a cache.

With all that in mind, here's the API I'm proposing for WXT 1.0:

// wxt/storage
export const storage: WxtStorage = ...;

export interface WxtStorage {
  getItem<T>(key: string): Promise<T | null>;
  getMeta<T extends Record<string, unknown>>(key: string): Promise<T>;
  getItems(keys: string[]): Promise<Array<{ key: string, value: any }>>;
  setItem<T>(key: string, value: T | null): Promise<void>;
  setMeta<T extends Record<string, unknown>>(key: string, fields: T | null): Promise<void>;
  setItems(values: Array<{ key: string, value: any }>): Promise<void>;
  removeItem<T>(key: string): Promise<void>;
  removeMeta<T>(key: string, fields?: string[]): Promise<void>;
  removeItems<T>(keys: string[]): Promise<void>;
  watch<T>(key: string, cb: WatchCallback<T>): Unwatch;
  defineItem<T>(key: string, options?: WxtStorageItemOptions): WxtStorageItem<T>;
  snapshot(base: string): Promise<any>;
  restoreSnapshot(base: string, data: any): Promise<void>;
}

export interface WxtStorageItem<T> {
  getItem(): Promise<T>;
  setItem(value: T | null): Promise<void>;
  setMeta<TMeta extends Record<string, unknown>>(value: TMeta): Promise<void>;
  removeItem(): Promise<void>;
  removeMeta(fields?: string[]): Promise<void>;
  watch(cb: WatchCallback<T>): Unwatch;
};

export interface WxtStorageItemOptions<T> {
  defaultValue?: T;
  version?: number;
  migrations?: Record<number, ((oldValue: any) => any)>;
};

export type WatchCallback<T> = (newValue: T, oldValue: T) => void;
export type Unwatch = () => void;

Edit 1: I removed comments since the function names are pretty self-explanatory. I also added the snapshot and restoreSnapshot functions.
Edit 2: Renamed onChange to watch and Unsubscribe to Unwatch.

@aklinker1
Copy link
Collaborator Author

New storage API was released in v0.13.0. Feel free to create new issues if you have problems or suggestions.

@dvlden
Copy link
Contributor

dvlden commented Dec 28, 2023

Oooh, very interesting. Dropping dependency sounds good, especially cause they did not care about our improvement. Plenty of dangling open PR's.

I just updated to v0.13.0 and going through refactor.

Got a question tho. I see everywhere literal examples of storing single primitive value per key in the storage. Would you advice to have a single storage item that stores an object instead?

Thinking about it, it's the way I used storage with my custom wrapper in the v2 of my extension. So now while developing v3, I do wonder why everyone is showing it as primitive values in storage only?

Since you have a lot of knowledge regarding development and extensions, please let me know. Would you advice me to use single source of truth basically or split it into multiple storage items?

I love the update!

––– UPDATE –––

Also, how performant is setting watcher per key vs single watcher and switch statement inside per key, as it was with unstorage?

@aklinker1
Copy link
Collaborator Author

aklinker1 commented Dec 28, 2023

Would you advice to have a single storage item that stores an object instead?

@dvlden When working with storage, I never use a single key with an object containing multiple fields that need to be set independently for two reasons:

  1. Avoid race conditions with other functions setting a different field in the same object
  2. Watch listeners are fired more than necessary - because listeners are based on top-level keys, listeners will fire when changing any value in the object, when you usually only care about one specific value

That's not to say you can't store objects. But you should only store objects that are updated as a single "value" all at once instead of updating individual parts of the object.

For example, if you have 5 different settings you need to store in your extension, I would store those 5 in separate keys, not a single "preferences" key, because they are updated one at a time. I don't have to get the latest value from storage before updating a single field of that object. I can just set the new value without worrying about that race condition

But if you are saving auth tokens for the logged in user, maybe a token and refresh token, those could be stored in a single "auth" key because they're never updated independently.

Also, how performant is setting watcher per key vs single watcher and switch statement inside per key, as it was with unstorage?

I decided to change this because I thought the old implementation might be a classic example of premature optimization. I've never measured the performance of the two approaches, so I don't have an answer for that.

Extension storage is implemented in native code in all browsers, so my thinking is that it's probably easier to let the native code manage the listeners.

If you are experiencing performance issues related to storage, I'll optimize it then.

@KnightYoshi
Copy link
Contributor

Here's what I'm thinking considering the feedback in this thread, my personal experience, and research into other libraries:

1. I like `unstorage`'s API and patterns
   
   * While `getItem` is verbose compared to `get`, it is fairly standard. It also allows for obvious plural forms of the functions, like `getItems`. It also allows for other setters, like `setMeta`, which could be useful in versioning fields.
   * The `:` separated keys are a nice approach, and using the `local:`/`session:`/`sync:`/`managed:` prefixes have worked well

2. Change listener will report the new and old values

3. I like the idea of creating a `StorageItem` from [`webext-storage`](https://www.npmjs.com/package/webext-storage), but recognize that it's not something everyone would use
   
   * I would prefer this over adding a storage schema to define the keys and their types, because that leads to circular imports
   * Migrations is always something I've wanted from a storage API, this could be a good

4. I've never had performance issues with reading and writing directly to storage, so I don't see a reason to add a cache.

With all that in mind, here's the API I'm proposing for WXT 1.0:

// wxt/storage
export const storage: WxtStorage = ...;

export interface WxtStorage {
  getItem<T>(key: string): Promise<T | null>;
  getMeta<T extends Record<string, unknown>>(key: string): Promise<T>;
  getItems(keys: string[]): Promise<Array<{ key: string, value: any }>>;
  setItem<T>(key: string, value: T | null): Promise<void>;
  setMeta<T extends Record<string, unknown>>(key: string, fields: T | null): Promise<void>;
  setItems(values: Array<{ key: string, value: any }>): Promise<void>;
  removeItem<T>(key: string): Promise<void>;
  removeMeta<T>(key: string, fields?: string[]): Promise<void>;
  removeItems<T>(keys: string[]): Promise<void>;
  watch<T>(key: string, cb: WatchCallback<T>): Unwatch;
  defineItem<T>(key: string, options?: WxtStorageItemOptions): WxtStorageItem<T>;
  snapshot(base: string): Promise<any>;
  restoreSnapshot(base: string, data: any): Promise<void>;
}

export interface WxtStorageItem<T> {
  getItem(): Promise<T>;
  setItem(value: T | null): Promise<void>;
  setMeta<TMeta extends Record<string, unknown>>(value: TMeta): Promise<void>;
  removeItem(): Promise<void>;
  removeMeta(fields?: string[]): Promise<void>;
  watch(cb: WatchCallback<T>): Unwatch;
};

export interface WxtStorageItemOptions<T> {
  defaultValue?: T;
  version?: number;
  migrations?: Record<number, ((oldValue: any) => any)>;
};

export type WatchCallback<T> = (newValue: T, oldValue: T) => void;
export type Unwatch = () => void;

Edit 1: I removed comments since the function names are pretty self-explanatory. I also added the snapshot and restoreSnapshot functions. Edit 2: Renamed onChange to watch and Unsubscribe to Unwatch.

I'd love a Migration API. It's something that I had tried to bake into my extension, but didn't quite get it right — tbf I was new to extensions at the time and tried a quick implementation within a week while developing the rest of it.

@aklinker1
Copy link
Collaborator Author

aklinker1 commented Dec 29, 2023

@KnightYoshi Try it out in v0.13.0 and let me know what you think :)

Here are the docs for setting them up: https://wxt.dev/guide/storage.html#versioning-and-migrations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants