hooks icon indicating copy to clipboard operation
hooks copied to clipboard

fix(createUseStorageState): batched updates

Open miaolq opened this issue 2 years ago • 11 comments

[中文版模板 / Chinese template]

🤔 This is a ...

  • [ ] New feature
  • [x] Bug fix
  • [ ] Site / documentation update
  • [ ] Demo update
  • [ ] TypeScript definition update
  • [ ] Bundle size optimization
  • [ ] Performance optimization
  • [ ] Enhancement feature
  • [ ] Internationalization
  • [ ] Refactoring
  • [ ] Code style optimization
  • [ ] Test Case
  • [ ] Branch merge
  • [ ] Other (about what?)

🔗 Related issue link

fix: https://github.com/alibaba/hooks/issues/2389

useLocalStorageState: Call setState(function) twice, the result is incorrect.

// Currently state is increased by 1 instead of 2 after click.
const [state, setState] = useLocalStorageState('key', { defaultValue: 0 })
const click = () => {
    setState(prev => prev + 1)
    setState(prev => prev + 1)
}

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • [x] Doc is updated/provided or not needed
  • [x] Demo is updated/provided or not needed
  • [x] TypeScript definition is updated/provided or not needed
  • [x] Changelog is provided or not needed

miaolq avatar Feb 27 '23 12:02 miaolq

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 27 '23 12:02 CLAassistant

该 PR 对应的钩子为 useLocalStorageState/useSessionStorageState,其旧的行为(该 PR 之前的行为)是:

const [name, setName] = useLocalStorageState<string>('key', {
  defaultValue: '0',
});

setName(name + '1');
setName(name + '2');
setName(name + '3');

console.log(name);
// 输出:'03',行为和 useState 钩子相同  

以及:

const [name, setName] = useLocalStorageState<string>('key', {
  defaultValue: '0',
});

setName(prev => prev + '1');
setName(prev => prev + '2');
setName(prev => prev + '3');

console.log(name);
// 输出:'03',行为和 useState 钩子不同

采用了该 PR 之后,useLocalStorageState/useSessionStorageState 的行为(下文简称“新的行为”)如下:

const [name, setName] = useLocalStorageState<string>('key', {
  defaultValue: '0',
});

setName(name + '1');
setName(name + '2');
setName(name + '3');

console.log(name);
// 输出:'03',行为和 useState 钩子相同  

以及:

const [name, setName] = useLocalStorageState<string>('key', {
  defaultValue: '0',
});

setName(prev => prev + '1');
setName(prev => prev + '2');
setName(prev => prev + '3');

console.log(name);
// 输出:'0123',行为和 useState 钩子相同

综上,

  • 旧的行为中,状态更新时,无论更新函数传值还是回调,更新总是“批量的”(多次合并为一次),和 useState 行为不完全相同
  • 新的行为中,状态更新时,行为和 useState 完全相同

所以,这个钩子最初被设计的行为是怎样的呢?如果新的行为是对的话,那么这个 PR 感觉会带来破坏性更改啊?(毕竟行为有很大的改变) @crazylxr 见哥 来看下,已经初步验证

liuyib avatar Feb 27 '23 14:02 liuyib

emm,我建议这个到 v4 的时候再做改动,现在确实会有 break change

crazylxr avatar Feb 28 '23 01:02 crazylxr

@liuyib @crazylxr 看了下此 PR 的修复方案,解决了批量更新行为和setState不一致的问题👍。但是存在一个细节不够完美,状态值和storage的值没有保持同步。这是因为使用 useEffect( storage.setItem(state), [state] ) 去同步 state -> storage 导致的。这个问题在此 https://github.com/alibaba/hooks/pull/2390 的解决方案中不会出现。 对比demo:https://stackblitz.com/edit/stackblitz-starters-zmdjte?file=src%2FlocalStorage.ts

Damon0820 avatar Dec 06 '23 15:12 Damon0820

@Damon0820 按照react的理念,传递给setState的函数应该是纯函数;setState执行后,真实的state在下次render时才变化,所以在useEffect( storage.setItem(state), [state] )中改变storage值也合理。

miaolq avatar Dec 07 '23 06:12 miaolq

@miaolq 嗯,要再综合考虑下

Damon0820 avatar Dec 07 '23 09:12 Damon0820

看了下文档。react对 Component、 initialState 、updater 都要求是纯函数。当前 initialState 的具体实现为 getStoredValue 方法,内部读取了外部变量的值storage?.getItem(key);,storage 的值理论上可能在外部其他地方变化,保证不了相同的输入,一定有相同的输出。所以getStoredValue 方法不是纯函数。不知道理解的对不对。 如果 initialState 不是纯函数,需要思考下有无其他更好的方案替代

Damon0820 avatar Dec 08 '23 02:12 Damon0820

看了下文档。react对 Component、 initialState 、updater 都要求是纯函数。当前 initialState 的具体实现为 getStoredValue 方法,内部读取了外部变量的值storage?.getItem(key);,storage 的值理论上可能在外部其他地方变化,保证不了相同的输入,一定有相同的输出。所以getStoredValue 方法不是纯函数。不知道理解的对不对。 如果 initialState 不是纯函数,需要思考下有无其他更好的方案替代

这个文档里看不到哪里指出了 initialState 也得是纯函数啊,截图示意下?Component 尽量是纯函数,这算业界公认;updater 是纯函数你这里一提,想了下还可以理解。initialState 纯不纯函数感觉意义不大,原因见下文:

参照文档: image React 也说了,有些情况确实没法保证使用纯函数。 尤其是这句: image

这里 initialState 里获取初始值,和上图推荐的“最后手段”,感觉是一个道理的。比如,不在 initialState 里获取初始值,把代码做如下更改:

const [state, setState] = useState(getStoredValue);

useUpdateEffect(() => {
  setState(getStoredValue());
}, [key]);

改成:

const [state, setState] = useState();

useEffect(() => {
  setState(getStoredValue());
}, [key]);

(上面的伪代码应该是等价的)这不又成了 React 文档里提到的“最后手段”,所以 initialState 没必要保证纯函数。

另外,我的理解,initialState 只会执行一次,即使从外部读取 localStorage,在组件多次渲染过程中,initialState 拿到的结果也只有一次,不会导致多次 render 渲染的 jsx 不一致,这样来说也没必要保证纯函数。

如果有错误,还请指出~

liuyib avatar Dec 08 '23 07:12 liuyib

抱歉,文档位置不准确,关于 initialState 是纯函数的说明在 setState api 这里。 image

按照 react 的理念,initialState是纯函数,当前的实现不满足纯函数。理论上,不是纯函数,都有潜在 bug 风险,在这边这个理论依然成立。因为该 hook 可能在不同的地方多次被调用,你不知道在哪个地方、哪个环节 storage 的值会被更改。对于使用该 hook 的同个组件,相同的输入,可能会有不同的输出。但是实际上,没有人会去改 storage 的值,所以实际用起来几乎没有 bug,即使开启了 <StrictMode>。同理,https://github.com/alibaba/hooks/pull/2390 的 updater 有类似的情况。 当然,可以把 initialState, updater 内读取 storage 的逻辑放到“最后手段” (useEffect),让 initialState, updater 是纯函数更加符合标准。但是这边使用 “最后手段” 去做 storage 的值同步,会存在一些的问题:

  • 互相同步值滞后。在下一次渲染时,才会将 storage 和 state 同步一致。
  • 将 storage 的值同步给 state 时,导致二次渲染。

感觉这里需要综合考虑下是否有必要保证 initialState, updater 是纯函数。 不知是否理解正确,望指出~

Damon0820 avatar Dec 11 '23 02:12 Damon0820

当然,可以把 initialState, updater 内读取 storage 的逻辑放到“最后手段” (useEffect),让 initialState, updater 是纯函数更加符合标准。但是这边使用 “最后手段” 去做 storage 的值同步,会存在一些的问题:

  • 互相同步值滞后。在下一次渲染时,才会将 storage 和 state 同步一致。
  • 将 storage 的值同步给 state 时,导致二次渲染。

是这样呢。确实按照 React 的理念应该保证这仨是纯函数,你是对的。但是感觉 100% 追求逻辑上符合理论,这个 hook 就废了,正如你说的会存在这些问题,,,,感觉还是实践大于理论些。先放这吧,回头其他大佬们有时间了找他们聊聊 最后,感谢你的建议哈 ❤️

liuyib avatar Dec 11 '23 06:12 liuyib

是这样呢。确实按照 React 的理念应该保证这仨是纯函数,你是对的。但是感觉 100% 追求逻辑上符合理论,这个 hook 就废了,正如你说的会存在这些问题,,,,感觉还是实践大于理论些。先放这吧,回头其他大佬们有时间了找他们聊聊 最后,感谢你的建议哈 ❤️

嗯啊,看看大佬们的建议。若有结论了同步一下哈~

Damon0820 avatar Dec 11 '23 08:12 Damon0820