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

fix(createUseStorageState): batched updates #2082

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

miaolq
Copy link

@miaolq miaolq commented Feb 27, 2023

[中文版模板 / Chinese template]

🤔 This is a ...

  • New feature
  • 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: #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. ⚠️

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

@CLAassistant
Copy link

CLAassistant commented Feb 27, 2023

CLA assistant check
All committers have signed the CLA.

@liuyib liuyib self-requested a review February 27, 2023 14:21
@liuyib
Copy link
Collaborator

liuyib commented Feb 27, 2023

该 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 liuyib added this to In progress in ahooks tasks via automation Feb 27, 2023
@liuyib liuyib moved this from In progress to Review in progress in ahooks tasks Feb 27, 2023
@crazylxr
Copy link
Collaborator

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

@crazylxr crazylxr added the v4 label Feb 28, 2023
@liuyib liuyib removed the question label Feb 28, 2023
@liuyib liuyib mentioned this pull request Mar 1, 2023
18 tasks
@crazylxr crazylxr moved this from Review in progress to v4 in ahooks tasks Mar 9, 2023
@Damon0820
Copy link

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

@miaolq
Copy link
Author

miaolq commented Dec 7, 2023

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

@Damon0820
Copy link

@miaolq 嗯,要再综合考虑下

@Damon0820
Copy link

Damon0820 commented Dec 8, 2023

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

@liuyib
Copy link
Collaborator

liuyib commented Dec 8, 2023

看了下文档。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 不一致,这样来说也没必要保证纯函数。

如果有错误,还请指出~

@Damon0820
Copy link

Damon0820 commented Dec 11, 2023

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

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

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

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

@liuyib
Copy link
Collaborator

liuyib commented Dec 11, 2023

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

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

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

@Damon0820
Copy link

Damon0820 commented Dec 11, 2023

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

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

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

Successfully merging this pull request may close these issues.

【bug】createUseStorageState连续多次setState(pre => xx)的表现和 useState 的不一致
5 participants