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

Optimize validate() behavior when validation disabled #75

Merged
merged 4 commits into from
Mar 7, 2022

Conversation

Luncher
Copy link
Contributor

@Luncher Luncher commented Mar 3, 2022

When the state is disabled, the validator should not be executed, and the state validation result (state.validation) should not change.

src/types.ts Outdated
@@ -42,7 +42,7 @@ export interface Validatable<T, TValue = T> {
validated: boolean
validationDisabled: boolean
validate(): Promise<ValidateResult<TValue>>

validateResult: ValidateResult<TValue>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这个为啥要加到 interface Validatable 上?state 的使用者并不需要读或者写它吧?

Copy link
Collaborator

@nighca nighca Mar 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* If state is disabled, the result of the last validation is obtained.
* If you need to clear the validation result, please use the reset function.
*/
@computed get validateResult(): ValidateResult<TValue> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

建议 private

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

尝试过这么加,不过被我自己否决了

@@ -152,6 +163,10 @@ export default class FieldState<TValue> extends Disposable implements Composible
* Fire a validation behavior.
*/
async validate(): Promise<ValidateResult<TValue>> {
if (this.validationDisabled) {
return this.validateResult
Copy link
Collaborator

@lzfee0227 lzfee0227 Mar 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIP: 我突然觉得 disabled 的定位应该重新明确一下…
除了看不见之外,还意味着什么?
比如“残留”着上次的状态以待后续潜在的重新 enabled 后接着用?
比如 disabled 的时候 set reset (以及后面的 touched)这些接口该如何理解?
比如一个 disabled 的 state 作为子 state 存在的时候,父 state 的 value 应该能拿到它的值吗?
诸如此类,等等

比如我就会想:
disabled 的时候要不要不清 error?
如果不清,那么转为 enabled 的那一刻,应该清了重新等 validate 或干脆不 active 吗?
如果都不清,那么 disable when 变更的心智模型就跟 validators 变更有区别了,这确实无所谓吗?

Copy link
Collaborator

@nighca nighca Mar 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

我们的表单状态模型是一棵 state 树,每个节点都是一个 state

在我自己的理解里,关于 disabled 的最直接的诉求是这样的:被 disabled 的 state,就像从当前 state 树上被摘掉了一样;原因很简单,因为这个 feature 对标的就是表单中的条件输入(conditional input)的场景,而这个场景对应的 input,就像是从当前 UI 树上被摘掉了一样

因此在理想的情况下

  1. 它的校验状态、结果不再反映到最终整体的校验状态、结果里
  2. 如果整个 state 树要做校验(validate()),它并不会随之去执行自己的校验
  3. 它的 value 不体现到最终 state 树的 value

其中,1 已经做到了,2 对应对应晨哥这个 PR 里的改动;至于 3,考虑到我们希望提供类型安全的 value,可预见的会比较麻烦,而且对表单页面的开发者意义不大,因为多给一个值几乎总是不会有问题的,他不去消费就好了,所以我理解 3 可以不去管

这样的话

就像从当前 state 树上被摘掉了一样

就不是特别严谨,严谨一点说是:被 disabled 的 state,就像从被当前 state 树上藏起来了一样;这个也很好理解,就像我们希望不展示一块界面的时候,也可能不是真的把它从 UI 树上摘掉,而是把它通过 css 藏起来

基于以上的前提,你上边提到的问题几乎都能得到一个自然的答案:

  • disabled 的时候 set reset (以及后面的 touched)这些接口该如何理解:就那么理解,你是在设置/操作一个被藏起来的状态,就像你操作一块被藏起来的界面一样
  • 比如一个 disabled 的 state 作为子 state 存在的时候,父 state 的 value 应该能拿到它的值吗:上面针对 value 说明过
  • disabled 的时候要不要不清 error:不用,没有必要,如果需要可以业务自己去做
  • 如果不清,那么转为 enabled 的那一刻,应该清了重新等 validate 或干脆不 active 吗:不用特别处理,保留原状态就好
  • 如果都不清,那么 disable when 变更的心智模型就跟 validators 变更有区别了,这确实无所谓吗:无所谓,本来这俩就不是一样的概念..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

就像从被当前 state 树上摘掉了一样

这你是从哪里引用到的…

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

自己说的,自己引用..

@nighca nighca changed the base branch from master to v2.x March 4, 2022 03:27
@nighca nighca closed this Mar 4, 2022
@nighca nighca reopened this Mar 4, 2022
Copy link
Collaborator

@nighca nighca left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤸‍♂️

@nighca nighca merged commit 1a1b6c4 into qiniu:v2.x Mar 7, 2022
@nighca nighca changed the title Optimize disable validate Optimize validate() behavior when validation disabled Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants