-
Notifications
You must be signed in to change notification settings - Fork 29
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
refactor(Project): extract auto-save logic from startEditing
#906
Conversation
aofei
commented
Sep 13, 2024
- Moved auto-save logic into dedicated methods.
- Improved code organization and testability.
- Fixed Occasional failures in auto-save related tests #880.
- Moved auto-save logic into dedicated methods. - Improved code organization and testability. - Fixed goplus#880. Signed-off-by: Aofei Sheng <aofei@aofeisheng.com>
The PR environment is ready, please check the PR environment [Attention]: This environment will be automatically cleaned up after a certain period of time., please make sure to test it in time. If you have any questions, please contact the builder team. |
await flushPromises() | ||
await vi.advanceTimersByTimeAsync(1000) // wait for changes to be picked up | ||
await flushPromises() | ||
autoSaveToCloud?.() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已经调用了 startAutoSaveToCloud
之后,还需要外部去直接触发对 autoSaveToCloud
的调用吗?通过加个 sprite 之类的操作来间接触发会不会更接近实际的逻辑?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
已经调用了 startAutoSaveToCloud
之后,会有 watcher 在负责监听 game files 的变动,所以不会有外部场景需要主动调用 autoSaveToCloud
。
这里把 autoSaveToCloud
暴露出来的主要目的是为了在 autoSaveMode
被切换时(online <-> offline)方便主动触发。在之前这个需求是通过 watch autoSaveMode
来实现的。
另外一个把 autoSaveToCloud
暴露出来的好处是,我们可以在测试中模拟自动流程了,因为在测试中依赖 watcher 是比较痛苦的,没有一个机制能来确保 watcher 被稳定触发。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
因为在测试中依赖 watcher 是比较痛苦的,没有一个机制能来确保 watcher 被稳定触发。
是指 watcher 的触发是异步的,所以不好确定它触发完的时机吗?
我之前是这样的
import { nextTick } from 'vue'
doSomeChange() // cause watcher to be triggered
await nextTick()
doCheckWatcherResult()
这个应该是可靠的
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
因为在测试中依赖 watcher 是比较痛苦的,没有一个机制能来确保 watcher 被稳定触发。
是指 watcher 的触发是异步的,所以不好确定它触发完的时机吗?
我之前是这样的
import { nextTick } from 'vue' doSomeChange() // cause watcher to be triggered await nextTick() doCheckWatcherResult()这个应该是可靠的
是的,我刚调试了一下,nextTick()
的确可以做到确保 watcher 已被触发。
但是,依赖 await nextTick()
来确保 watcher 被触发完的场景应该只适用于同步 callback?以 startAutoSaveToCloud
中的 watcher 为例:
this.addDisposer(
watch(
() => this.exportGameFiles(),
async (files, _, onCleanup) => {
let cancelled = false
onCleanup(() => (cancelled = true))
const filesHash = await hashFiles(files)
if (cancelled) return // avoid race condition and ensure filesHash accuracy
this.filesHash = filesHash
if (this.hasUnsyncedChanges) this.autoSaveToCloud?.()
},
{ immediate: true }
)
)
单只是 await nextTick()
,应该只能确保代码执行到 const filesHash = await hashFiles(files)
就会返回(因为 dom 更新完了?)。所以我们的场景 await nextTick()
之后应该不能立即做检查,比如 expect(project.hasUnsyncedChanges).toBe(true)
并不成立。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
噢是的,你这里在 autoSaveToCloud()
前还有个 await hasFiles()
..那是挺麻烦的,那就按现在的做法来吧