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(form): fix form multiple bind visibility #1531

Merged
merged 2 commits into from
Oct 14, 2022

Conversation

milabi
Copy link
Contributor

@milabi milabi commented Sep 22, 2022

当有嵌套Object property时且嵌套的属性中定义了visibleIf,_bindVisibility被重复调用并订阅执行setVisible。当visibleIf发生变化时将重新触发resetValue多次。

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Application (the showcase website) / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Sorry, something went wrong.

fn(child) 会调用property._bindVisibility()最终调用到forEachChild,无需重复执行
rootProperty._bindVisibility() 只有根Property执行即可,嵌套的Object Property如果也调用,会造成各属性的_bindVisibility重复执行,visibleIf被重复订阅,引起resetValue被重复执行。
@milabi milabi changed the title Fix form multiple bind visibility fix(form): Fix form multiple bind visibility Sep 22, 2022
@ng-alain-bot
Copy link
Contributor

ng-alain-bot commented Sep 22, 2022

Preview is ready!

@codecov
Copy link

codecov bot commented Sep 22, 2022

Codecov Report

Merging #1531 (c740e19) into master (2fb2d2b) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1531      +/-   ##
==========================================
- Coverage   95.60%   95.60%   -0.01%     
==========================================
  Files         272      272              
  Lines        8436     8435       -1     
  Branches     1655     1573      -82     
==========================================
- Hits         8065     8064       -1     
  Misses        310      310              
  Partials       61       61              
Impacted Files Coverage Δ
packages/form/src/model/form.property.ts 97.96% <ø> (-0.03%) ⬇️
packages/form/src/model/form.property.factory.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@cipchk
Copy link
Member

cipchk commented Oct 2, 2022

是否呈现会依赖目标,但是目标是不确定的,基于此问题,能否提供一个复现的示例?

@milabi
Copy link
Contributor Author

milabi commented Oct 6, 2022

以下是一个小demo,每次点启用,将触发三次的asyncData的调用。

https://stackblitz.com/edit/angular-e5vkg9?file=src%2Fapp%2Fdelon-abc.module.ts,src%2Fapp%2Fapp.component.ts

@cipchk
Copy link
Member

cipchk commented Oct 9, 2022

这种状态我认为是合理的,因为我们无法确认是否需要依赖异步数据来做变更动作。

建议本地做好缓存处理。

@milabi
Copy link
Contributor Author

milabi commented Oct 9, 2022

不管是否依赖异步数据来做变更动作,三次的reset调用是否有必要。如果这个嵌套再深一层将会有更多次的订阅,而事件的起因是因为依赖的visibleIf只发生一次改变。我们是否有必要因为一次改变而订阅三次,这三次所做的事件是一样的,并没有不同,仅仅是因为多订阅了。

@cipchk cipchk changed the title fix(form): Fix form multiple bind visibility fix(form): fix form multiple bind visibility Oct 9, 2022
@cipchk cipchk merged commit a4e62ef into ng-alain:master Oct 14, 2022
cipchk added a commit that referenced this pull request Oct 30, 2022
@cipchk cipchk mentioned this pull request Oct 30, 2022
3 tasks
cipchk added a commit that referenced this pull request Oct 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants