alibaba / formily

📱🚀 🧩 Cross Device & High Performance Normal Form/Dynamic(JSON Schema) Form/Form Builder -- Support React/React Native/Vue 2/Vue 3
https://formilyjs.org/
MIT License
11.36k stars 1.48k forks source link

fix: multi source update #3892

Open hchlq opened 1 year ago

hchlq commented 1 year ago

Before submitting a pull request, please make sure the following is done...

Please do not delete the above content


What have you changed?

fix #3837

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01 :tada:

Comparison is base (b9ab509) 96.59% compared to head (d97dfa0) 96.60%.

:exclamation: Current head d97dfa0 differs from pull request most recent head 7f859a3. Consider uploading reports for the commit 7f859a3 to get more accurate results

Additional details and impacted files ```diff @@ Coverage Diff @@ ## formily_next #3892 +/- ## ================================================ + Coverage 96.59% 96.60% +0.01% ================================================ Files 152 152 Lines 6695 6725 +30 Branches 1869 1824 -45 ================================================ + Hits 6467 6497 +30 - Misses 200 228 +28 + Partials 28 0 -28 ``` | [Impacted Files](https://app.codecov.io/gh/alibaba/formily/pull/3892?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=alibaba) | Coverage Δ | | |---|---|---| | [packages/reactive/src/types.ts](https://app.codecov.io/gh/alibaba/formily/pull/3892?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=alibaba#diff-cGFja2FnZXMvcmVhY3RpdmUvc3JjL3R5cGVzLnRz) | `100.00% <ø> (ø)` | | | [packages/reactive/src/autorun.ts](https://app.codecov.io/gh/alibaba/formily/pull/3892?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=alibaba#diff-cGFja2FnZXMvcmVhY3RpdmUvc3JjL2F1dG9ydW4udHM=) | `100.00% <100.00%> (ø)` | | | [packages/reactive/src/reaction.ts](https://app.codecov.io/gh/alibaba/formily/pull/3892?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=alibaba#diff-cGFja2FnZXMvcmVhY3RpdmUvc3JjL3JlYWN0aW9uLnRz) | `96.89% <100.00%> (+0.03%)` | :arrow_up: | | [packages/reactive/src/tracker.ts](https://app.codecov.io/gh/alibaba/formily/pull/3892?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=alibaba#diff-cGFja2FnZXMvcmVhY3RpdmUvc3JjL3RyYWNrZXIudHM=) | `100.00% <100.00%> (ø)` | | ... and [9 files with indirect coverage changes](https://app.codecov.io/gh/alibaba/formily/pull/3892/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=alibaba)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

janryWang commented 1 year ago

这个改动量有点大,仔细说说你的思路吧

hchlq commented 1 year ago

改动量还好,兼容历史版本的。

思路:调整 _boundary 的类型从 number 改为 Map<Target, Key>,用于存储更新源的信息,避免嵌套更新多个源时被忽略。target 和 key 在 runReactions 时保存到 reaction 中,执行 batchEnd 之前,保存当前的更新源,batchEnd 后有更新的,如果更新的 target 和 key 已经在 Map 中并且匹配上了,那么忽略更新(历史版本),否则说明是其他更新源,执行 tracker 重新进行依赖收集。

改动:

  1. 更改 Reaction 的 _boundary 类型,从 number 改为 Map<Target, Key>。
  2. 增加 Reaction 的 _updateKey 和 _updateTarget 两个类型,在 runReactions 时保存到 reaction 中。
  3. 使用了_boundary 的 observer,即 autorun 和 tracker,均做了调整。
janryWang commented 1 year ago

https://github.com/alibaba/formily/issues/3666 这个问题我感觉也能一起修,你可以构造一下测试用例看看,这是他的 reactive 用例 https://codesandbox.io/s/loving-wilson-578l5g?file=/index.js

gwsbhqt commented 1 year ago

@hchlq

因为你的 _updateKey 是单个变量,并且在 runReactions 中不断替换。如果出现间接循环那这个 _boundary 的过滤就会失效。 考虑 _updateKey 出现的序列可能为:undefined -> A -> B -> A -> B -> ...

单测例子:

test('repeat execute autorun cause by deep indirect dependency', () => {
  const obs: any = observable({ aa: 1, bb: 1, cc: 1 })
  const fn = jest.fn()
  const fn2 = jest.fn()
  const fn3 = jest.fn()
  autorun(() => fn((obs.aa = obs.bb + obs.cc)))
  autorun(() => fn2((obs.bb = obs.aa + obs.cc)))
  autorun(() => fn3((obs.cc = obs.aa + obs.bb)))
  expect(fn).toBeCalledTimes(4)
  expect(fn2).toBeCalledTimes(4)
  expect(fn3).toBeCalledTimes(3)
})

35a82e20-c93b-49ee-813c-a9286cbaea62

hchlq commented 1 year ago

处理了 _updateKey 间隔更新的情况,_boundary 的类型 Map<any, any> 改为 Map<any, Set<any>>

gwsbhqt commented 1 year ago

@hchlq

因为你的 _updateKey 是单个变量,并且在 runReactions 中不断替换。所以如果在一个 batch 中存在多个 key 的值更新,那这个 _boundary 的过滤就会失效。

test('repeat execute autorun cause by deep indirect dependency', () => {
  const obs: any = observable({ aa: 1, bb: 1, cc: 1 })
  const fn = jest.fn()
  const fn2 = jest.fn()
  const fn3 = jest.fn()
  autorun(() => fn((obs.aa = obs.bb + obs.cc)))
  autorun(() => fn2((obs.bb = obs.aa + obs.cc)))
  autorun(() => fn3((obs.cc = obs.aa + obs.bb)))

  expect(fn).toBeCalledTimes(4)
  expect(fn2).toBeCalledTimes(4)
  expect(fn3).toBeCalledTimes(3)

  fn.mockClear()
  fn2.mockClear()
  fn3.mockClear()

  batch(() => {
    obs.aa = 1
    obs.bb = 1
    obs.cc = 1
  })

  expect(fn).toBeCalledTimes(2)
  expect(fn2).toBeCalledTimes(2)
  expect(fn3).toBeCalledTimes(2)
})

74169a4d-9b2e-4d3a-8df7-1888b53f2aaf

hchlq commented 1 year ago

这个 case 我处理一下

gwsbhqt commented 1 year ago

@hchlq

autorun对待递归来源时需要至多执行一次,你的方案失去了递归响应能力

test('autorun should trigger it self at most once', () => {
  const obs = observable({ aa: 1, bb: 1 })

  autorun(() => {
    obs.aa = obs.bb + 1
    obs.bb = obs.aa + 1
  })

  expect(obs.aa).toBe(4)
  expect(obs.bb).toBe(5)

  autorun(() => {
    obs.aa = obs.bb + 1
    obs.bb = obs.aa + 1
  })

  expect(obs.aa).toBe(10)
  expect(obs.bb).toBe(11)
})
image
hchlq commented 1 year ago

这个我知道,#3666 一样的问题

janryWang commented 1 year ago

@hchlq 改的怎么样了呢

hchlq commented 1 year ago

@janryWang 这块我还在想哈。

还没有更好的方案,目前是解决了多个源更新的问题,但是还有个问题是「递归来源时需要至多执行一次」这个问题还没有解决

hchlq commented 1 year ago

@janryWang @gwsbhqt 「递归来源时需要至多执行一次」这个应该还需要讨论下,看了下 formliy 之前的实现和参考了 vue 的 reactivity,autorun 中执行的更新都是会忽略的,因此上面的 testcase 预期应该是这样的:

test('multiple update should trigger only one', () => {
  const obs = observable({ aa: 1, bb: 1 })

  autorun(() => {
    obs.aa = obs.bb + 1
    obs.bb = obs.aa + 1
  })

  expect(obs.aa).toBe(2)
  expect(obs.bb).toBe(3)

  autorun(() => {
    obs.aa = obs.bb + 1
    obs.bb = obs.aa + 1
  })

  expect(obs.aa).toBe(6)
  expect(obs.bb).toBe(7)
})
hchlq commented 1 year ago

@janryWang

3666 这个问题在当前的 autorun 实现中比较难去解决,和嵌套更新冲突了,但是给出的例子中可以通过其他方法绕过,再给出的 例子 中,将 delete fieldB.value 去掉即可。

难解决的原因:在 batch 中执行到 fieldB.value = "fieldB" 时 -> 保存 value 到 cache 中并执行 delete fieldB.value ,此时触发 autorun fieldC.visible (第一次更新,但是 batchEnd 没执行) -> 继续执行 batch 中 fieldA.value = "fieldA" -> fieldB.visible = true -> fieldB.value = fieldB.cache (第二次更新)为嵌套的更新,因此忽略执行

MeetzhDing commented 9 months ago

@hchlq @janryWang 这个问题现在有什么进展吗?同遇到了这个问题,而且感觉还挺容易触发的