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

[RMBWEB-2780] Support for structured validation results #87

Merged
merged 22 commits into from
May 29, 2022

Conversation

Luncher
Copy link
Contributor

@Luncher Luncher commented May 9, 2022

See details in #82

@Luncher Luncher changed the title [RMBWEB-2780] Support for structured validation results [WIP][RMBWEB-2780] Support for structured validation results May 9, 2022
@Luncher Luncher changed the title [WIP][RMBWEB-2780] Support for structured validation results [RMBWEB-2780] Support for structured validation results May 10, 2022
src/adapter/v2.ts Outdated Show resolved Hide resolved
src/adapter/v2.ts Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/utils.spec.ts Show resolved Hide resolved
src/state.ts Outdated Show resolved Hide resolved
@@ -25,10 +25,12 @@ export interface IState<V> {
activated: boolean
/** Current validate status. */
validateStatus: ValidateStatus
/** The error info of validation. */
/** The error info of validation, ErrorObject type will be filled with ErrorObject.message. */
Copy link
Collaborator

@nighca nighca May 11, 2022

Choose a reason for hiding this comment

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

建议不要放在 error & ownError 上说明;不需要用 rawError 的人就不需要接触 ErrorObject 之类的概念是最好

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这里是从这边 state 同步过来的

不需要用 rawError 的人就不需要接触 ErrorObject 之类的概念是最好

意思是, error & ownError 实现上包含了 ErrorObejct, 但是这边不说吗?..

Copy link
Collaborator

Choose a reason for hiding this comment

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

嗯,我感觉这里不说应该问题不大,毕竟类型上也会反映它只会是 string | undefined;当他真的想要通过 validator 返回 ErrorObejct 的时候,那时他能找到对应的文档就好

src/types.ts Outdated Show resolved Hide resolved
src/types.ts Outdated
export type ValidationError = string | undefined
export type ValidationRawError = ErrorObject | ValidationError
Copy link
Collaborator

Choose a reason for hiding this comment

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

ValidationRawErrorValidationResult 统一成一个东西会不会好点?这俩其实很接近,多一个概念不如少一个?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

从概念上讲,ValidationResultValidationResp 这种比较接近,而不是 ValidationRawError, 后者只是前者的子集。和在一起的话,会不会有理解上的困难,之前的实现好像有意的区分,ResultError 的区别:

this.setError(isValid(result) ? undefined : result)

Copy link
Collaborator

Choose a reason for hiding this comment

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

从概念上讲,ValidationResultValidationResp 这种比较接近

ValidationResp 是啥?

之前的实现好像有意的区分,ResultError 的区别

是的,ResultError 是两个概念;不过 RawErrorResult 可以做成一个概念?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ResultError 是两个概念

当然可以不区分这两个概念我觉得会更好,只是因为从方便 validator 实现的角度来说,需要允许它 return boolean | string | undefined | null,而从方便 error 消费的角度来说会希望它的取值是 string | undefined;所以这边拆了两个概念来达到这个目的

如果我们评估觉得,对 validator 来说只允许它 return string | undefined 也还算方便,且有比较好的兼容历史(validator)代码的方法,那么我会倾向于把 ResultError 也合成一个概念的

Copy link
Contributor Author

@Luncher Luncher May 12, 2022

Choose a reason for hiding this comment

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

如果我们评估觉得,对 validator 来说只允许它 return string | undefined 也还算方便,且有比较好的兼容历史(validator)代码的方法

我估计这样对用户负担比较大。譬如:

  return new FormState({
    name: new FieldState('').validators(v => !v && '用户名不能为空'),
    email: new FieldState('').validators(
      val => !val && '邮箱不能为空',
      val => !mailReg.test(val) && '请输入有效的邮箱地址'
    )
  })

类似这种写法还是蛮多的。如果换成:

  return new FormState({
    name: new FieldState('').validators(v => !v  ?  '用户名不能为空' : undefined),
    email: new FieldState('').validators(
      val => !val ? '邮箱不能为空' : undefined,
      val => !mailReg.test(val) ? '请输入有效的邮箱地址' : undefined
    )
  })

相对使用体验没有那么好。

按照目前的使用习惯,用户其实已经养成了 “零” 值表示验证通过了:

export function isValid(result: ValidationResult): result is '' | null | undefined | false {
  return !result
}

从方便 validator 实现的角度来说我觉得这个还是留着比较好。

Copy link
Collaborator

Choose a reason for hiding this comment

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

如果我们评估觉得,对 validator 来说只允许它 return string | undefined 也还算方便

哦我不是要现在评估,我们也不可能现在把对于 null | false 的支持去掉(这是 breaking);我是指,如果我们将来评估得出“支持 null | false 不重要”这一结论,那么我也会倾向把 ResultError 合成一个概念

这边是两个事情:

  1. Result & Error 概念合并
  2. Result & RawError 概念合并

1 我们肯定不会现在去做的,也不用现在去评估;之所以上边提到 1 是因为 1 跟 2 的动力是一样的:概念越少理解起来越简单

@@ -97,6 +99,10 @@ States will not be auto-validated until it becomes **activated**. And they will

`ownError` & `hasOwnError` are special fields especially for composed states. You can check details about them in issue [#71](https://github.com/qiniu/formstate-x/issues/71).

### Raw Error

The state's own raw error info, regardless of child states. The difference compared to `ownError` is that it contains the type of `ErrorObject`. You can check details about them in issue [#82](https://github.com/qiniu/formstate-x/issues/82).
Copy link
Collaborator

Choose a reason for hiding this comment

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

如果像 #87 (comment) 这里说的统一成一个概念;这里我们就可以说,raw error 就是 validator 返回的结果,error 是对 validator 返回的结果加工后的值;这样跟 raw 的含义也比较贴合

Copy link
Contributor Author

@Luncher Luncher May 12, 2022

Choose a reason for hiding this comment

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

总的来说,我感觉 result -> rawError -> error 这样感觉好点,

对于 result : falsy 表示校验通过
对于 rawError & error: undefined 表示通过(没有错误)
rawErrorerror 多了 ValidationErrorObject 类型

Copy link
Collaborator

Choose a reason for hiding this comment

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

我知道这样对你(实现的人)来说是清晰的;只是对用的人来说,他就需要被迫多理解一个概念

如果我们回去 #82 看诉求,会发现它的诉求是不拆分 Result & RawError 就能满足的

另外如果我们从使用者的视角来看:

interface FooError {
  foo: true
  message: string
}

function validateFoo() {
  return { foo: true, message: 'xxx' }
}

function isFooError(err): err is FooError {
  return err && err.foo === true
}

const fooState = new FieldState('').withValidators(validateFoo)

if (isFooError(fooState.rawError)) {
  // ...
}

以上是使用者使用 rawError 的 feature 的时候可能会写的代码;比较容易理解的是:fooState.rawError 就是他的 validateFoo 返回的东西,所以他可以直接用 isFooError 去检查并消费之;而如果你跟他说,fooState.rawError 不完全是 validateFoo 返回的东西,这里边还有一个转换逻辑在,就不是特别友好了——他如果严谨的话,需要去了解下这个转换逻辑,才敢放心地消费 fooState.rawError

Copy link
Contributor Author

Choose a reason for hiding this comment

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

而如果你跟他说,fooState.rawError 不完全是 validateFoo 返回的东西,这里边还有一个转换逻辑在..

我们给 validator 的实现提供了方便(返回 falsy 表示验证通过),那么这个转换逻辑天然就是存在的,用户能够有预期的,用户消费的 hasErrorerror 都实际上隐含了这种转换。 而如果现在在实现 rawError 的时候把 validator 的返回值原封不动给到用户,用户是不是反而更近困惑。

另一方面把 result 的概念跟 rawError 等价/合并起来,我感觉不太恰当,因为我们提出 rawError 的概念是为了体现出 ValidationErrorObjectfalsy 值毕竟不是一个 “错误”..

Copy link
Collaborator

@nighca nighca May 13, 2022

Choose a reason for hiding this comment

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

是因为我们默认这个字段叫 rawError 了,它名字里带了 error 所以会认为它是“错误”;如果我给它取名叫 validationResult 呢:

interface IState {
  validationResult: ValidationResult // 所有 validator 结果的 mix
  error: ValidationError // 基于 validationResult 处理得到
}

它也能满足 #82 的需求

src/utils.ts Outdated
/** If validation result is valid */
export function isValid(result: ValidationResult): result is '' | null | undefined | false {
return !result
export function isValidationPassed(result: ValidationResult): result is undefined {
Copy link
Collaborator

Choose a reason for hiding this comment

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

: result is undefined 不对吧?

Copy link
Collaborator

Choose a reason for hiding this comment

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

需要注意下:

rawError: { message: '' }

的话,hasError 应该是 true 还是 false;如果是 true,src/state.ts 那里 class BaseStatehasError 逻辑也需要调整下,应该跟这里逻辑也是相关的

Copy link
Contributor Author

@Luncher Luncher May 13, 2022

Choose a reason for hiding this comment

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

我感觉:
1、对于 validator 的返回值,falsy 表示没有错误
2、对于 hasErrorthis.error !== undefined 表示没有错误

这样是不是好点?

所以返回 { message: '' } 表示验证失败。

Copy link
Collaborator

Choose a reason for hiding this comment

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

所以 rawError: { message: '' } 的时候,认为没有错误(检查通过)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

所以 rawError: { message: '' } 的时候,认为没有错误(检查通过)?

我觉得不通过, 合理

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我打算限制一下这种情况,如果出现这种情况就直接给用户报错了,即:强制用户输入 message,这样能省不少事。不过在类型上不太好限制。 @nighca @lzfee0227 你们觉得咋样。

Copy link
Collaborator

@nighca nighca May 16, 2022

Choose a reason for hiding this comment

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

意思是,预期 ErrorObjectmessage 一定是一个非空字符串对吧?我没问题

不过在类型上不太好限制

这个问题不大,可以在文档里说

如果出现这种情况就直接给用户报错了

如果文档里明确说了的话,那不去报错也没啥问题,毕竟这不是一个容易犯的错误

Copy link
Contributor Author

@Luncher Luncher May 16, 2022

Choose a reason for hiding this comment

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

不过感觉,它(hasError)的值也从 rawError / validationResult 直接得到要比从 error 直接得到更好

如果从 rawError/ validationResult 取的话。hasError 是不是得换个其他名字比较恰当,比如 hasRawError。当前这个 hasError 这么实现还有个好处是,它能同时表示 package errorown error,即:FormState/ArrayState/FieldState 等都能通过这个属性表示有没有错误。

而更接近上层的展示逻辑

hasErrorerror 对应,我们的 error 定位就是展示用的,比如 bindFormState 消费的就是这个 error,所以我觉得没问题的..

Copy link
Collaborator

@nighca nighca May 16, 2022

Choose a reason for hiding this comment

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

P.S. 以上我提到的“文档”,默认都是指广义的文档概念,包括 docs/ 中的内容,也包括代码中对字段、定义的注释说明(这些内容同样会被使用者阅读)

Copy link
Collaborator

Choose a reason for hiding this comment

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

当前这个 hasError 这么实现还有个好处是,它能同时表示 package errorown error,即:FormState/ArrayState/FieldState 等都能通过这个属性表示有没有错误。

这个没太理解,因为现在 has error 表达的都是 has packaged error,而不是 has own error

hasErrorerror 对应,我们的 error 定位就是展示用的,比如 bindFormState 消费的就是这个 error,所以我觉得没问题的..

对,所以我现在感觉 hasErrorerror 更底层了;如果 error 是 error info 用于展示文案用途的“投影”,那么提供对应于这个投影的 has error 就意义不大了;用户更关心的应该是,是否存在错误(是否 pass),而不是是否存在一份用于展示的错误文案(尽管这俩在结果上很接近)

src/utils.ts Outdated
@@ -22,7 +41,7 @@ export function asyncResultsAnd(asyncResults: Array<Promise<ValidationResult>>):
let validResultCount = 0
asyncResults.forEach(asyncResult => asyncResult.then(result => {
// return error if any result is invalid
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIP: 注释的说法最好也调整下,“result valid” -> “validation passed”

Copy link
Collaborator

@lzfee0227 lzfee0227 left a comment

Choose a reason for hiding this comment

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

感觉有点复杂… = =

src/state.ts Outdated
@observable protected _error: ValidationRawError

@computed get rawError() {
return this.disabled ? undefined : this._error
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return this.disabled ? undefined : this._error
return this.disabled ? undefined : this._error


it('normalizeError should work well', () => {
expect(normalizeError(undefined)).toBe(undefined)
expect(normalizeError('')).toBe('')
Copy link
Collaborator

Choose a reason for hiding this comment

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

这是符合预期的吗

Comment on lines 397 to 399
state.validate()

await delay()
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIP: 类似这种为什么不直接 await state.validate()

src/utils.ts Outdated
}

export function isErrorObject(err: any): err is ValidationErrorObject {
return err != null && typeof err === 'object' && 'message' in err
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里要对齐空字符串的行为吗?
就是类似这种 && err.message !== ''

src/state.ts Outdated
})()
}

@computed protected get validateResult(): ValidateResult<V> {
return (
this.error
this.error !== undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

这样行为是不是变了啊?
以前对外接口 setError('') 的时候是不是算没 error 的

src/state.ts Outdated
@@ -11,7 +11,7 @@ export abstract class BaseState extends Disposable implements Pick<
abstract error: ValidationError

@computed get hasError() {
return !!this.error
return this.error !== undefined
Copy link
Collaborator

Choose a reason for hiding this comment

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

不会产生 breaking change 吗?

Copy link
Collaborator

Choose a reason for hiding this comment

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

不会 breaking,因为之前 error 的值不会是 '' | null | false

Copy link
Collaborator

@nighca nighca May 13, 2022

Choose a reason for hiding this comment

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

哦算上 setError 的使用姿势的话,确实有可能是 '',那目测会 break

P.S. 这个使用姿势需要补个 test case 覆盖下 @Luncher

Copy link
Contributor Author

Choose a reason for hiding this comment

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

如果没有开启 strictNullCheck 的项目,是不是有可能会设置 setError(null) 啊..

Copy link
Collaborator

Choose a reason for hiding this comment

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

@Luncher 不能这么说,那样是使用姿势有问题了

不使用 TS 的项目还有可能 setError(123) 呢,我们也没必要考虑这种情况

Copy link
Contributor Author

Choose a reason for hiding this comment

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

setError('') 我觉得也不是一个正常的姿势,我看了下,pilifusion 都没有这种场景的。虽然兼容了感觉还是给个 warning 好了?

Copy link
Collaborator

Choose a reason for hiding this comment

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

虽然兼容了感觉还是给个 warning 好了?

我没问题;不过如果给 warning 了,那最好也调整下文档,让使用者通过文档能知道我们说的“error info” 的概念,其值作为字符串时应当是非空的

对了,我在想,是不是应该给使用者设置 raw error(而不是 error)的能力才是更合理的?

describe('should work well with resolved error object', () => {
it('should work well with sync resolved', async () => {
const state = new FieldState('').withValidator(
_ => ({ message: 'error-object-msg'})
Copy link
Collaborator

Choose a reason for hiding this comment

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

好多这种少了右空格的 😂

src/state.ts Outdated
IState, 'ownError' | 'hasOwnError' | 'error' | 'hasError' | 'validateStatus' | 'validating' | 'validated'
> {
IState, 'rawError' | 'ownError' | 'hasOwnError' | 'hasError' | 'validateStatus' | 'validating' | 'validated'
> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

为啥这里加缩进?

src/types.ts Outdated
@@ -39,8 +40,8 @@ export interface IState<V = unknown> {
hasError: boolean
/** The state's own error info, regardless of child states. */
ownError: ValidationError
/** Ihe state's own error info, includes ValidationErrorObject error type, regardless of child states. */
rawError: ValidationRawError
/** The state's validation result, is the same as the return value of the validator, regardless of child states. */
Copy link
Collaborator

Choose a reason for hiding this comment

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

is the same as the return value of the validator

这个描述不准确?参考 https://qiniu.github.io/formstate-x/concepts/validator#validation-result

/** Result of validation. */
export type ValidationResult =
  string
  | null
  | undefined
  | false

/** Return value of validator. */
export type ValidatorReturned = 
  ValidationResult
  | Promise<ValidationResult>

/** A validator checks if given value is valid. **/
export type Validator<T> = (value: T) => ValidatorReturned

src/utils.ts Show resolved Hide resolved
@@ -97,6 +99,10 @@ States will not be auto-validated until it becomes **activated**. And they will

`ownError` & `hasOwnError` are special fields especially for composed states. You can check details about them in issue [#71](https://github.com/qiniu/formstate-x/issues/71).

### Raw Error

The state's validation result, is the same as the return value of the `validator`, regardless of child states. The difference compared to `ownError` is that it contains the type of `ValidationErrorObject`. You can check details about them in issue [#82](https://github.com/qiniu/formstate-x/issues/82).
Copy link
Collaborator

@nighca nighca May 23, 2022

Choose a reason for hiding this comment

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

Nit: 后边有时间的时候,最好是在文档 Guide/Advanced 里补一下对应的 case(可以不在这个 PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK..

src/state.ts Outdated
return this.disabled ? undefined : this._error
}

@computed get ownError() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个是不是能挪到 BaseState 上了啊?因为看起来不管是什么样的 state,ownError = normalizeError(rawError) 这个逻辑都是成立的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

不太明确 BaseState 的定位,我看 BaseState 上一些属性,跟 ValidatableState 关系也挺大的

Copy link
Collaborator

@nighca nighca May 23, 2022

Choose a reason for hiding this comment

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

BaseState 抽的是对所有 state 都成立的逻辑,现在 formstate-x 代码中所有对 interface IState 的实现都直接或间接地继承了它

ValidatableState 是校验逻辑的抽象,适用于那些有自己的校验逻辑/过程的 state

举个例子,TransformedState 继承了 BaseState 而没有继承 ValidatableState,是因为它没有“自己的校验逻辑/过程”,而是依赖的其包裹的 state($)的校验;TransformedState 不需要维护自己的 validators,也没有自己的 validate status,也不需要自己去维护一份单独的 error 信息

Copy link
Collaborator

@nighca nighca May 23, 2022

Choose a reason for hiding this comment

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

BaseState 抽的是对所有 state 都成立的逻辑

基于这个标准看的话,现在 hasError 放在这上边就不太合适了,因为现在 hasError 的实现逻辑(hasError = !isPassed(rawError))并不是对所有 state 都成立的

Copy link
Collaborator

Choose a reason for hiding this comment

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

假如我们有 packaged raw error,那么我们其实是能实现出这样一份对所有 state 都适用的逻辑的:

ownError = normalizeError(rawError)
error = normalizeError(packagedRawError)
hasError = !isPassed(packagedRawError)

这样不同的 state,它们去实现各自的 rawError & packagedRawError 就好,逻辑会相对清晰一点

不过现在我们没有 packagedRawError,也没有一个可靠的从 rawError 推出 hasError 的逻辑,所以那些 override 了 error 的地方都需要再去 override 一遍 hasError;基于这个前提,我觉得 #87 (comment) 这里提到的

不过感觉,它(hasError)的值也从 rawError / validationResult 直接得到要比从 error 直接得到更好

就不太对了;既然我们已经通过 throw 确保了 error object 的 message 一定不是空字符串,那么 hasError = !!error 的逻辑就还成立(而且是对所有的 state 成立),那走这个逻辑会对实现更方便?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

既然我们已经通过 throw 确保了 error object 的 message 一定不是空字符串,那么 hasError = !!error 的逻辑就还成立(而且是对所有的 state 成立),那走这个逻辑会对实现更方便?

#87 (comment)

我之前在这里就说了啊..,现在要把实现改回去吗?

Copy link
Collaborator

Choose a reason for hiding this comment

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

#87 (comment)

我之前在这里就说了啊..

那边我没看懂你的意思..

现在要把实现改回去吗?

我倾向改回去,如果确实 hasError = !!error 这个逻辑还成立,而且基于这个逻辑确实更方便的话

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ba4b45e
done

src/state.ts Outdated
*/
@observable protected _error: ValidationError
@observable private _error: ValidationResult
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: 这个字段名改成 validationResult 会稍好点?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

其实我觉得 setError 跟 _error 配对挺好的,现在只改了这里一个, setError 暂时也动不了,是不是很奇怪?

Copy link
Collaborator

Choose a reason for hiding this comment

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

如果我们关于这一点是一致的:这里(_error & setError)叫 validation result 比叫 error 更准确;那么我们应该把尽可能多的地方改成准确的,而让为了保持兼容而留的、不够准确的尽可能少?

Copy link
Collaborator

@nighca nighca May 23, 2022

Choose a reason for hiding this comment

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

甚至可以考虑:

// 这里是反映内在逻辑的
private validationResult: ValidationResult
private setValidationResult(v: ValidationResult) {
  this.validationResult = v
}

// 这里是 API 层面保持兼容的
public setError(v: ValidationResult) {
  return this.setValidationResult(v)
}

这样就比较清晰了

Copy link
Contributor Author

Choose a reason for hiding this comment

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

如果这么改只是内部实现有这么一个概念,我感觉最主要的是让用户知道通过 setError 设置的值,跟 validation result 是等价的。
所以我觉得,在你上述的基础上把 setValidationResult 改成 public、把 setError 标识成 deprecated 会不会更合理一点

Copy link
Collaborator

Choose a reason for hiding this comment

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

setError 标识成 deprecated 会不会更合理一点

我觉得不用;你现在对用户侧的概念还是“error”(raw error 也是基于 error 的概念);所以在用户的感受上,setErrorrawError 应该是更像配对的;如果你把 setValidationResult 暴露出去,那你也应该把 rawError 命名调整成 validationResult

return this.$.rawError
}

@override override get hasError() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: 这个 override 好像不需要,BaseState 提供了一份 hasError 实现:!isPassed(this.rawError),跟你这 override 后的逻辑效果应该是一样的

Copy link
Contributor Author

Choose a reason for hiding this comment

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

如果 transformoriginalState 是一个 AbstractFormState,就有可能不对了。

Copy link
Collaborator

Choose a reason for hiding this comment

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

哦对,我把 raw error 下意识理解成 packaged 了..

expect(normalizeError('')).toBe(undefined)
expect(normalizeError('foo')).toBe('foo')
expect(normalizeError({ message: 'mewo' })).toBe('mewo')
expect(normalizeError(Error('mewo2'))).toBe('mewo2')
Copy link
Collaborator

Choose a reason for hiding this comment

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

需要 normalizeError({ message: '' }) 的 case

src/utils.ts Outdated
export function isErrorObject(err: any): err is ValidationErrorObject {
if (err != null && typeof err === 'object' && 'message' in err) {
if (!err.message) {
throw new Error('ValidationErrorObject message property cannot be empty')
Copy link
Collaborator

@nighca nighca May 23, 2022

Choose a reason for hiding this comment

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

在这(isErrorObject)抛错不合理吧..放到 setError 去抛可能会好一点,甚至 normalizeError 也比这里好

@@ -25,7 +25,7 @@ class Upgrader<T extends v2.ComposibleValidatable<unknown, V>, V> extends BaseSt

@computed get value() { return this.stateV2.value }
@computed get touched() { return this.stateV2.dirty }
@computed get ownError() {
@override override get ownError() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这边之所以需要 override,恰恰反映是逻辑上实现反了?应该是基于 rawError 得到 ownError,这里做成了基于 ownError 得到 rawError

src/utils.ts Outdated
@@ -14,13 +19,8 @@ export function normalizeError(result: ValidationResult): ValidationError {
return result
}

export const inValidErrorObjectMsg = 'ValidationErrorObject message property cannot be empty'

export function isErrorObject(err: any): err is ValidationErrorObject {
if (err != null && typeof err === 'object' && 'message' in err) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

这里是不是最好再加个 typeof err.message === 'string'

Copy link
Collaborator

Choose a reason for hiding this comment

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

P.S. 如果加了 typeof err.message === 'string',好像 'message' in err 也可以省了

src/state.ts Outdated
@@ -54,23 +56,23 @@ export abstract class ValidatableState<V> extends BaseState implements IState<V>
@observable activated = false

/**
* The original error info of validation.
* The original return value of validation.
Copy link
Collaborator

Choose a reason for hiding this comment

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

这个注释也改一下?

Suggested change
* The original return value of validation.
* The original validation result.

@@ -52,7 +53,7 @@ export class DebouncedState<S extends IState<V>, V = ValueOf<S>> extends Validat

@override override get ownError() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

在实现 DebouncedState 的时候,也应该去通过 override rawError 来实现,而不是 override ownError

#87 (comment) 这里所述,ownError = normalizeError(rawError) 这个逻辑对于 DebouncedState 也应该是成立的?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

没注意这里有个 override,我看怎么改下。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

031f8a1
done

@@ -25,10 +25,9 @@ class Upgrader<T extends v2.ComposibleValidatable<unknown, V>, V> extends BaseSt

@computed get value() { return this.stateV2.value }
@computed get touched() { return this.stateV2.dirty }
@computed get ownError() {
@computed get rawError() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

多了个空格?以及第 30 行也多了个

/** Infomation synced from original state */
type OriginalInfo<V> = Pick<IState<V>, 'activated' | 'touched' | 'error' | 'ownError' | 'hasError'>
/** Information synced from original state */
type OriginalInfo<V> = Pick<IState<V>, 'activated' | 'touched' | 'error' | 'ownError' | 'hasError' | 'rawError'>
Copy link
Collaborator

Choose a reason for hiding this comment

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

ownErrorhasError 都可以干掉了

if (this.disabled) return undefined
if (this.rawError) return normalizeError(this.rawError)
return this.original.ownError
if (this.validationResult) return this.validationResult
Copy link
Collaborator

Choose a reason for hiding this comment

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

if (!isPassed(this.validationResult)) 会比 if (this.validationResult) 更严谨吗?

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
Copy link
Collaborator

nighca commented May 23, 2022

我这没问题了 @lzfee0227 你要再看下吗?

src/utils.ts Outdated
return result.message
}

if (result === false || result == null || result == '') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

特地不 === '' 的吗?

src/utils.ts Outdated
import { Validator, ValidationResult, ValidatorReturned } from './types'
import { Validator, ValidatorReturned, ValidationError, ValidationErrorObject, ValidationResult } from './types'

export const inValidErrorObjectMsg = 'ValidationErrorObject message property cannot be empty'
Copy link
Collaborator

Choose a reason for hiding this comment

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

invalid 是一个单词吧

Comment on lines +24 to +26
return true
}
return false
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIP: 特地改成这样有啥说法么 = =

Copy link
Contributor Author

Choose a reason for hiding this comment

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

没懂..

Copy link
Collaborator

Choose a reason for hiding this comment

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

一开始 pr 里是 return a && b && c 差不多这样的
后面特地改成

if (a && b && c) {
  return true
}
return false

后面有比前面更好吗…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

你不说我都忘记之前的 isErrorObject 的实现是

return a && b && c 

了。。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

如果是:a && b && c、返回值可能就不是 true/false 了。

Copy link
Collaborator

Choose a reason for hiding this comment

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

一开始我也是这么想的
但是看了一下你的实现,也不是这情况…
另外如果是为了防止出现那种状况
我会选择标记一下返回类型为 boolean

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我试了下,: err is ValidationErrorObject 这种,只能返回 boolean

Copy link
Collaborator

Choose a reason for hiding this comment

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

对喔,那就更没问题了

src/state.ts Outdated

/** Extraction for some basic features of State */
export abstract class BaseState extends Disposable implements Pick<
IState, 'ownError' | 'hasOwnError' | 'error' | 'hasError' | 'validateStatus' | 'validating' | 'validated'
IState, 'rawError' | 'ownError' | 'hasOwnError' | 'hasError' | 'validateStatus' | 'validating' | 'validated'
Copy link
Collaborator

Choose a reason for hiding this comment

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

弱问这里的 'error' 不用留着吗?

> {

abstract rawError: ValidationResult
Copy link
Collaborator

Choose a reason for hiding this comment

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

rawErrorValidationResult 是对的吗?
后面还有类似的这种

Copy link
Contributor Author

@Luncher Luncher May 25, 2022

Choose a reason for hiding this comment

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

rawError 对 ValidationResult 是对的吗?

是预期的

后面还有类似的这种

啥?

Copy link
Collaborator

Choose a reason for hiding this comment

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

没啥,是预期的就好
我只是看到 error 对 result 感觉怪怪的

Copy link
Collaborator

@lzfee0227 lzfee0227 left a comment

Choose a reason for hiding this comment

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

l g t m

@nighca nighca merged commit ec9e6cf into qiniu:master May 29, 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