[Storage] When `getValue` returns `defaultValue` it doesn't actually save it

[Bas950]

Feature Request

Either always save the defaultValue in storage, or add a setting for the defaultValue to be saved in storage when it get's returned.

Is your feature request related to a bug?

N/A

Additional context

import { v4 as uuid } from "uuid";

storage.defineItem<string>("local:identifier", { defaultValue: uuid() });

[veryCrunchy]

A defaultGetValue and defaultSetValue would be nice, with defaultValue being an alias for defaultGetValue defaultSetValue would work the same as defaultGetValue, only it saves the value upon being created when calling .getValue(), or when calling .setValue() without passing any args.

[Timeraa]

From what I see and understand a comment should be added to defaultValue to indicate that it's solely a return value and not the default value that the storage will be set to in chrome storage. I would expect migrate() to set it to the default value when run.

[aklinker1]

Yup, migrate would work. But a separate API for "constants" might make sense as well.

That said, something like a user ID probably shouldn't use defineItem. Since you never need to set it.

Instead, I'd recommend something like this:

import { Mutex } from 'async-mutex';
import nanoid from 'nanoid';

const userIdMutex = new Mutex():

export function getUserId(): Promise<string> {
  return userIdMutex.runExclusive(async () => {
    const id = await storage.getItem<string>('local:user-id');
    if (id) return id;

    const newId = nanoid();
    await storage.setItem('local:user-id', newId);
    return newId;
  })
}

This was typed up from memory on mobile, so there may be syntax errors or incorrect imports. Double check.

If I were to implement a feature like storage.defineConstant, this is how it would be implemented, doing everything I can to avoid race conditions.

[Bas950]

I think adding a defineConstant might not be a bad idea.

[aklinker1]

A defaultGetValue and defaultSetValue would be nice, with defaultValue being an alias for defaultGetValue defaultSetValue would work the same as defaultGetValue, only it saves the value upon being created when calling .getValue(), or when calling .setValue() without passing any args.

@veryCrunchy would something like this cover your idea? What you described above seems overly complex, and I'd prefer a simple "define constant" API instead.

[veryCrunchy]

Yes, that'd be great

[aklinker1]

@veryCrunchy @Bas950 Here's the docs for the new storage.defineConstant feature. Do you have any feedback on it? Want me to change the names or what the API looks like?

https://deploy-preview-827--creative-fairy-df92c4.netlify.app/guide/extension-apis/storage.html#defining-constant-items

[Bas950]

@veryCrunchy @Bas950 Here's the docs for the new storage.defineConstant feature. Do you have any feedback on it? Want me to change the names or what the API looks like?

https://deploy-preview-827--creative-fairy-df92c4.netlify.app/guide/extension-apis/storage.html#defining-constant-items

LGTM

[veryCrunchy]

What is stopping a user from modifying the "constant" value in localstorage and potentially breaking infrastructure? There should be some preventive measure in place, constants have to be constant.

[aklinker1]

What is stopping a user from modifying the "constant" value in localstorage and potentially breaking infrastructure?

There should be some preventive measure in place, constants have to be constant.

Nothing right now. I considered adding something, but that would suddenly require every set to get the value first. And I didn't think that was worth the potential impact to performance.

Would a different word than "constant" fit better?

[Timeraa]

What is stopping a user from modifying the "constant" value in localstorage and potentially breaking infrastructure? There should be some preventive measure in place, constants have to be constant.

You can't really prevent a user from changing it, you can only really "complicate it" by for example hashing it and checking for that hash but that's too much effort than its worth imo, it's still a "constant" if you clear your local storage it's gone anyway...

[aklinker1]

OK, so it seems like "constant" is the wrong word to use for this because it comes with a lot of baggage around preventing it from being changed. What about defineInitializedItem. defineReadonlyItem has the same naming problems as defineConstant... Other than that, I'm out of ideas lol. Maybe defineLazyItem?

Here's what Claude.ai has to say:

It also suggested defineLazyItem... But I don't know. It doesn't provide enough emphasis on not being able to set the value, which is why I liked "defineConstant"

defineComputedItem is interesting, but the value isn't always computed, which might confuse vue developers.

Overall... I still prefer defineConstant, and that the value is "constant by convention".

[aklinker1]

Maybe the solution is defineLazyItem, and allow setting the value? Then we don't have to worry about the connotation of "constant" if we provide the ability to set the value.

We use storage.defineLazyItem, and return a LazyStorageItem (instead of the StorageItem returned by storage.defineItem.) LazyStorageItem is a superset of StorageItem, which allows setting values, but it has a custom getter that uses a mutex to prevent race conditions (as shown above) AND it includes an init function if you want to initialized the value at some point.

export interface LazyStorageItem extends StorageItem {
  init(): Promise<void>;
}

function defineLazyItem(key, options) {
  const item = storage.defineItem(key, options);

  return {
    ...item,
    getValue: () => { ... },
    init: () => { ... },
  }
}

But then I might as well integrate it into the regular defineItem as an option...

export const userId = storage.defineItem("local:user-id", {
  init: () => globalThis.crypto.randomUUID()
});

Only downside to this approach is not having an init function on the storage item. Could use typescript magic to include it on the return value if you passed an init option, but I'd rather avoid complicated types like that.

[aklinker1]

Alright, after lots of thinking, I'm going to just add an init option to the existing storage.defineItem. It handles the original ask, is different from the defaultValue option. Note that it will not be lazy anymore, I'm going to run the init function immediately when the context starts up.

export const userId = storage.defineItem("local:user-id", {
  init: () => globalThis.crypto.randomUUID()
});

If someone passes both a default value AND init, the default value will basically have no effect.

[veryCrunchy]

I think defaultValue should be renamed to something that better describes its behaviour, like fallback/fallbackValue to avoid confusion. init is exactly what we need (and what I meant with the defaultSetValue), having it executed immediately if there is no value set, and maybe being able to execute it manually to regenerate and set the value. Which would be safest by exposing the function through an alias reInit and having init be private, to avoid confusion of having to manually initialize.

[aklinker1]

I think defaultValue should be renamed to something that better describes its behaviour, like fallback/fallbackValue to avoid confusion.

I don't want to remove defaultValue for now, that's a breaking change I don't want to make until the storage package is extracted from WXT. For now, I'll add fallback and just deprecate defaultValue, that's a better name.

init is exactly what we need (and what I meant with the defaultSetValue), having it executed immediately if there is no value set, and maybe being able to execute it manually to regenerate and set the value.

Haha yeah, I didn't understand what you were trying to say in the original comment, sorry about that. But we got there!

Which would be safest by exposing the function through an alias reInit and having init be private, to avoid confusion of having to manually initialize.

"Note that it will not be lazy anymore, I'm going to run the init function immediately when the context starts up." - Since it's not going to be lazy anymore, there shouldn't be any confusion around having to re-initialize the value.

Unless you think it should be lazy?

[veryCrunchy]

defaultValue should definitely not be removed, sorry for the confusion there.

Unless you think it should be lazy?

It should definitely not be lazy It would however be nice to be able to re initialize, if we want to update the value using the init function we passed

userId.reInitialize()

there shouldn't be any confusion around having to re-initialize the value.

If .init() would be exposed though, instead of .reInitialize(), it could add confusion about whether the storage item has to be manually initialized or not.

It might be easier if I create a pr?

[aklinker1]

I understand now. What is your use-case for needing to re-initialize a value?

Edit: I'm working on the code right now, and you can essentially re-initialize the value via:

await item.removeValue();
await item.getValue();

[aklinker1]

Here's the reworked docs using defineItem and an init option: https://deploy-preview-827--creative-fairy-df92c4.netlify.app/guide/extension-apis/storage.html#default-values

[aklinker1]

Released in v0.19.0!