uni-helper / uni-use

uni-app (vue3) 组合式工具集
MIT License
128 stars 16 forks source link

feat: add useStorage #31

Closed bobby169 closed 10 months ago

bobby169 commented 10 months ago

Description

Add useStorage, which is simpler and more direct compared to useStorageAsync and is used more often in daily scenarios.

ModyQyW commented 10 months ago

I think there's still a serious problem with not listening for storage changes. That's why I didn't introduce the useStorage method before, uni-app doesn't provide an API to handle this kind of listening.

Suppose a user uses useStorage to get the data with key test in page A, then opens page B, then uses useStorage to get and modify the data in page B, and then returns to page A. Is the data with key test held by the user in page A new or old? From the code, it looks like it's still old. I'm concerned that this will result in a bad experience.

What do you guys think? Will be nice to listen to your thoughts! @okxiaoliang4 @edwinhuish

bobby169 commented 10 months ago

I think there's still a serious problem with not listening for storage changes. That's why I didn't introduce the useStorage method before, uni-app doesn't provide an API to handle this kind of listening.

Suppose a user uses useStorage to get the data with key test in page A, then opens page B, then uses useStorage to get and modify the data in page B, and then returns to page A. Is the data with key test held by the user in page A new or old? From the code, it looks like it's still old. I'm concerned that this will result in a bad experience.

What do you guys think? Will be nice to listen to your thoughts! @okxiaoliang4 @edwinhuish

the stores/userInfo.js

export const userStore = useStorage("x-userStore", {
    num: 0,
    deep:{
       a: 1
    }
});

pageA

import { userStore } from "@/stores/userStore";
const toNext = () => {
  uni.navigateTo({
    url: "/pages/pageB",
  });
};

<button @click="toNext">userStore num is {{ userStore.num }}</button>
<button @click="toNext">userStore deep.a is {{ userStore.deep.a }}</button>

pageB

import { userStore } from "@/stores/userStore";
const toNext = () => {
  uni.navigateTo({
    url: "/pages/pageA",
  });
};
const change = () => {
    userStore.value.num += 1;
    userStore.value.deep.a += 1;
}
<button @click="change">userStore num ++ {{ userStore.num }}</button>
<button @click="change">userStore deep.a ++ {{ userStore.deep.a }}</button>
<button @click="toPrev"> to page A</button>

I have already tested the code above, and there are no issues. You can give it a try @ModyQyW . Thanks

okxiaoliang4 commented 10 months ago

我认为应该跟useStorageAsync共用一份核心代码,因为差别就只有是否是异步的,监听逻辑我认为也应该类似于useStorageAsync目前使用的拦截器,因为拦截器可以拦截到开发者直接使用uni storage进行更新的情况,并且在H5的时候可以考虑直接内部改用vueuse/core中的useStorage这样可以做到多个窗口同步数据

bobby169 commented 10 months ago

我认为应该跟useStorageAsync共用一份核心代码,因为差别就只有是否是异步的,监听逻辑我认为也应该类似于useStorageAsync目前使用的拦截器,因为拦截器可以拦截到开发者直接使用uni storage进行更新的情况,并且在H5的时候可以考虑直接内部改用vueuse/core中的useStorage这样可以做到多个窗口同步数据

The code in uni-use useStorageAsync is too outdated compared to the latest code in vueuse. The new useStorage I submitted actually aligns with the useStorage in vueuse. I think it's necessary to update the old useStorageAsync to make it consistent with the latest useStorage code.

edwinhuish commented 10 months ago

I think there's still a serious problem with not listening for storage changes. That's why I didn't introduce the useStorage method before, uni-app doesn't provide an API to handle this kind of listening.

Suppose a user uses useStorage to get the data with key test in page A, then opens page B, then uses useStorage to get and modify the data in page B, and then returns to page A. Is the data with key test held by the user in page A new or old? From the code, it looks like it's still old. I'm concerned that this will result in a bad experience.

What do you guys think? Will be nice to listen to your thoughts! @okxiaoliang4 @edwinhuish

Listen for the storage changes will cause very impactful on performance.

I think the code are fine, but need some impove:

  1. Add a key prefix, to make sure it won't confused with other keys which access directly to storage.
  2. Add debounce to write storage
  3. Store all values to a reactive/ref variable, for some case like below:

Page A


const store = useStorage('store_key', 'foo');

store.value = 'bar';

console.log(store.value); // bar

Page B


// Call useStorage with key 'store_key', will access directly from local variable in 'useStorage' context
const store = useStorage('store_key', 'foo');

console.log(store.value); // Here should be also 'bar'. 
edwinhuish commented 10 months ago

Also, I'm agree with @okxiaoliang4 , useStorageAsync and useStorage should be use same code. If useStorageAsync outdated, then renew it.

bobby169 commented 10 months ago

I think there's still a serious problem with not listening for storage changes. That's why I didn't introduce the useStorage method before, uni-app doesn't provide an API to handle this kind of listening. Suppose a user uses useStorage to get the data with key test in page A, then opens page B, then uses useStorage to get and modify the data in page B, and then returns to page A. Is the data with key test held by the user in page A new or old? From the code, it looks like it's still old. I'm concerned that this will result in a bad experience. What do you guys think? Will be nice to listen to your thoughts! @okxiaoliang4 @edwinhuish

Listen for the storage changes will cause very impactful on performance.

I think the code are fine, but need some impove:

  1. Add a key prefix, to make sure it won't confused with other keys which access directly to storage.
  2. Add debounce to write storage
  3. Store all values to a reactive/ref variable, for some case like below:

Page A

const store = useStorage('store_key', 'foo');

store.value = 'bar';

console.log(store.value); // bar

Page B

// Call useStorage with key 'store_key', will access directly from local variable in 'useStorage' context
const store = useStorage('store_key', 'foo');

console.log(store.value); // Here should be also 'bar'. 
  1. Add a key prefix, to make sure it won't confused with other keys which access directly to storage.

The key for userStorage is defined by the user themselves; we can't define its prefix, nor can we control it.

  1. Add debounce to write storage
  const { pause: pauseWatch, resume: resumeWatch } = pausableWatch(data, () => write(data.value), {
    flush,
    deep,
    eventFilter,
  });

function update(event?: StorageEventLike) {
    if (event && event.storageArea !== storage) return;

    if (event && event.key == null) {
      data.value = rawInit;
      return;
    }

    if (event && event.key !== key) return;

    pauseWatch();
    try {
      if (event?.newValue !== serializer.write(data.value)) data.value = read(event);
    } catch (error) {
      onError(error);
    } finally {
      // use nextTick to avoid infinite loop
      if (event) nextTick(resumeWatch);
      else resumeWatch();
    }
  }

The pausableWatch function in vueuse already allows for pauseWatch(). With frequent updates to a large amount of data in localStorage, there's no longer a need to worry, and no need for debounce anymore.

  1. Store all values to a reactive/ref variable, for some case like below:

I tested again, referring to your code, and there were no issues. The mini-program also passed the test. My code is as follows:

// pageA
const store = useStorage('store_key', 'foo');

const changeTest = () => {
  store.value = 'bar';
  console.info(store.value) // bar
};

const toNext = () => {
  uni.navigateTo({
    url: "/pages/pageB",
  });
};

<button @click="toNext">{{ store }}</button>
<view>{{ store }}</view>
//  pageB
const store = useStorage('store_key', 'foo');

const changeTest = () => {
  store.value = String(new Date().getTime());
  console.info(store.value) // bar
};

const toPrev = () => {
  uni.navigateBack({
    delta: 1,
  });
};

<button @click="toPrev">{{ store }}</button>
<view>{{ store }}</view>

Moreover, I believe the useStorage function should only be defined once in /stores/xx, similar to Pinia. It shouldn't be redundantly redefined across multiple pages with default values given. Even with such redundant definitions, the above code can still pass the test.

bobby169 commented 10 months ago

Also, I'm agree with @okxiaoliang4 , useStorageAsync and useStorage should be use same code. If useStorageAsync outdated, then renew it.

The code I submitted for useStorage is 90% similar to the latest code in vueuse's useStorage. Except for replacing UniStorage and custom events, the code is almost identical to the latest in vueuse. Why can't we let useStorage pass the test and stabilize before updating useStorageAsync? I think the closer we get to the latest version of vueuse, the fewer mistakes we can make.

bobby169 commented 10 months ago

我认为应该跟useStorageAsync共用一份核心代码,因为差别就只有是否是异步的,监听逻辑我认为也应该类似于useStorageAsync目前使用的拦截器,因为拦截器可以拦截到开发者直接使用uni storage进行更新的情况,并且在H5的时候可以考虑直接内部改用vueuse/core中的useStorage这样可以做到多个窗口同步数据

The problem with syncing many pages doesn't always need useInterceptor. Using custom events for data changes can also sync others pages. With uni.addInterceptor, we try to copy useEventListener(window, 'storage', e => Promise.resolve().then(() => read(e))). This is for updating localStorage in both sync, Sadly, uni.addInterceptor doesn't yet support sync methods like uni.setStorageSync(KEY, DATA).

But this kind of sync update is rarely needed, almost not important, Because users rarely directly update code in already defined localStorage, Update data can be exported from the stores, and always change data.value.xxx. which then triggers changes in data. Still, useStorageAsync would be better with useInterceptor.

edwinhuish commented 10 months ago

key prefix is something like:


const prefix = '__UNI_STORAGE__';

function useStorage( key: string ) {

  const keyWithPrefix = prefix + key;

  function read() {
    const raw = storage.getItem(keyWithPrefix);
    return JSON.parse(raw);
  }

  // Same for write()

  return { read }
}

When some on use this function in one page and unfortunately also access storage with same key in another page.

Page A

const store = useStorage('user');

// Another code....

Page B

// Set value to storage with same 'key' (At least it looks the same) will not affect the 'store' variable of page A
const user = storage.setItem('user',  'john'); 
edwinhuish commented 10 months ago

In my understanding, useStorage most important thing is "persistedstate", and should simplify the user's job. It will not like pinia, and shouldn't.

It should be something take and use. Also can be paired with pinia.

In your code way, it's better to use pinia, and it's already very reliable.

edwinhuish commented 10 months ago

I never use pausableWatch before.

After read the doc and code, it's not the one I mean Add debounce to write storage. The code of pausableWatch seems a little complicate. I prefer the normal debounce way ( setTimeout ), it's good enough and the important things is, it can effectively control the frequency of writing data to storage, it can effectively improve the performance.

My suggestion: if simple code can work well, don't import third part code. It will make the things complicate.

edwinhuish commented 10 months ago

The third point "Store all values to a reactive/ref variable", just I'm saying before, useStorage should be something simple and easy to use.

And should avoid possible mistake of user.

edwinhuish commented 10 months ago

I tested again, referring to your code, and there were no issues

Because you access the data with difference page, and reloaded data from storage. You can try it with same page but difference components.

bobby169 commented 10 months ago

In my understanding, useStorage most important thing is "persistedstate", and should simplify the user's job. It will not like pinia, and shouldn't.

It should be something take and use. Also can be paired with pinia.

In your code way, it's better to use pinia, and it's already very reliable.

You should check how to use useLocalStorage. It's chosen here because it's effective for data that stays stored. I know Pinia also has plugins for persistent storage. Otherwise, what's the point of this repository? Just to offer more options to users?

bobby169 commented 10 months ago

I tested again, referring to your code, and there were no issues

Because you access the data with difference page, and reloaded data from storage. You can try it with same page but difference components.

I've already said above, the problem isn't big, it's seldom used this way.

useLocalStorage must be used like Pinia. Otherwise, might as well directly use uin.storage.

useLocalStorage works like pinia. It's easier than pinia and keeps data persistent storage. That's why useLocalStorage is needed. So, it should be used like pinia.

ModyQyW commented 10 months ago

I think there's still a serious problem with not listening for storage changes. That's why I didn't introduce the useStorage method before, uni-app doesn't provide an API to handle this kind of listening. Suppose a user uses useStorage to get the data with key test in page A, then opens page B, then uses useStorage to get and modify the data in page B, and then returns to page A. Is the data with key test held by the user in page A new or old? From the code, it looks like it's still old. I'm concerned that this will result in a bad experience. What do you guys think? Will be nice to listen to your thoughts! @okxiaoliang4 @edwinhuish

the stores/userInfo.js

export const userStore = useStorage("x-userStore", {
    num: 0,
    deep:{
       a: 1
    }
});

pageA

import { userStore } from "@/stores/userStore";
const toNext = () => {
  uni.navigateTo({
    url: "/pages/pageB",
  });
};

<button @click="toNext">userStore num is {{ userStore.num }}</button>
<button @click="toNext">userStore deep.a is {{ userStore.deep.a }}</button>

pageB

import { userStore } from "@/stores/userStore";
const toNext = () => {
  uni.navigateTo({
    url: "/pages/pageA",
  });
};
const change = () => {
    userStore.value.num += 1;
    userStore.value.deep.a += 1;
}
<button @click="change">userStore num ++ {{ userStore.num }}</button>
<button @click="change">userStore deep.a ++ {{ userStore.deep.a }}</button>
<button @click="toPrev"> to page A</button>

I have already tested the code above, and there are no issues. You can give it a try @ModyQyW . Thanks

I think there are some misunderstandings. In pageB, it should be uni.navigateBack() but not uni.navigateTo.

ModyQyW commented 10 months ago

我认为应该跟useStorageAsync共用一份核心代码,因为差别就只有是否是异步的,监听逻辑我认为也应该类似于useStorageAsync目前使用的拦截器,因为拦截器可以拦截到开发者直接使用uni storage进行更新的情况,并且在H5的时候可以考虑直接内部改用vueuse/core中的useStorage这样可以做到多个窗口同步数据

我感觉 H5 直接内部改用 @vueuse/core 中的 useStorage 可能不太好,会带来额外的心理负担。

useStorageAsync 目前使用的拦截器不支持同步 api,所以这应该是目前最大的困难。

ModyQyW commented 10 months ago

我认为应该跟useStorageAsync共用一份核心代码,因为差别就只有是否是异步的,监听逻辑我认为也应该类似于useStorageAsync目前使用的拦截器,因为拦截器可以拦截到开发者直接使用uni storage进行更新的情况,并且在H5的时候可以考虑直接内部改用vueuse/core中的useStorage这样可以做到多个窗口同步数据

The code in uni-use useStorageAsync is too outdated compared to the latest code in vueuse. The new useStorage I submitted actually aligns with the useStorage in vueuse. I think it's necessary to update the old useStorageAsync to make it consistent with the latest useStorage code.

Good point. PR welcome.