Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
restruct normalize raw error
  • Loading branch information
Luncher committed May 12, 2022
commit f466115fec8eb4fa2b2ac7d6bc965f74eb570a2d
28 changes: 19 additions & 9 deletions src/adapter/v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,13 @@ import * as v2 from 'formstate-x-v2'
import { BaseState } from '../state'
import * as v3 from '..'
import Disposable from '../disposable'
import { isPromiseLike, normalizeError, normalizeRawError } from '../utils'

interface IV3StateFromV2<T extends v2.ComposibleValidatable<unknown, V>, V> extends v3.IState<V> {
/** The original ([email protected]) state */
$: T
}

type ExcludeErrorObject<V> = V extends v3.ErrorObject ? never : V extends Promise<infer R> ? Promise<ExcludeErrorObject<R>> : V

type Conditional<K> = K extends any ? ExcludeErrorObject<K> : never

// Omit ErrorObject
type LegacyValidator<T> = (V: T) => Conditional<ReturnType<v3.Validator<T>>>

class Upgrader<T extends v2.ComposibleValidatable<unknown, V>, V> extends BaseState implements IV3StateFromV2<T, V> {
constructor(private stateV2: T) {
super()
Expand Down Expand Up @@ -50,12 +44,12 @@ class Upgrader<T extends v2.ComposibleValidatable<unknown, V>, V> extends BaseSt
setForV2(this.stateV2, value)
}
reset() { this.stateV2.reset() }
withValidator(...validators: Array<LegacyValidator<V>>) {
withValidator(...validators: Array<v3.Validator<V>>) {
if (
isV2FieldState(this.stateV2)
|| isV2FormState(this.stateV2)
) {
this.stateV2.validators(...validators)
this.stateV2.validators(...portV2Validators(...validators))
return this
}
throwNotSupported()
Expand All @@ -72,6 +66,22 @@ class Upgrader<T extends v2.ComposibleValidatable<unknown, V>, V> extends BaseSt
}
}

function portV2Validators<V>(...validators: Array<v3.Validator<V>>): Array<v2.Validator<V>> {
const normalizeRet = (v: any) => (
normalizeError(normalizeRawError(v))
)
return validators.map(validator => {
return (value: V) => {
const returned = validator(value)
if (isPromiseLike(returned)) {
return returned.then(normalizeRet)
} else {
return normalizeRet(returned)
}
}
})
}

/** Converts [email protected] state to [email protected] state */
export function fromV2<T extends v2.ComposibleValidatable<unknown, unknown>>(stateV2: T): IV3StateFromV2<T, T['value']> {
return new Upgrader(stateV2)
Expand Down
4 changes: 2 additions & 2 deletions src/debouncedState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { action, computed, makeObservable, observable, override, reaction } from
import { FieldState } from './fieldState'
import { ValidatableState } from './state'
import { IState, ValidateStatus, ValueOf } from './types'
import { debounce, isErrorObject } from './utils'
import { debounce, normalizeError } from './utils'

const defaultDelay = 200 // ms

Expand Down Expand Up @@ -52,7 +52,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

if (this.disabled) return undefined
if (this._error) return isErrorObject(this._error) ? this._error.message : this._error
if (this._error) return normalizeError(this._error)
return this.original.ownError
}

Expand Down
6 changes: 3 additions & 3 deletions src/state.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { action, autorun, computed, makeObservable, observable, when } from 'mobx'
import { ValidationRawError, ValidationError, IState, Validation, ValidateResult, ValidateStatus, Validator } from './types'
import Disposable from './disposable'
import { applyValidators, isValid, isPromiseLike, isErrorObject } from './utils'
import { applyValidators, isPromiseLike, normalizeRawError, normalizeError } from './utils'

/** Extraction for some basic features of State */
export abstract class BaseState extends Disposable implements Pick<
Expand Down Expand Up @@ -63,7 +63,7 @@ export abstract class ValidatableState<V> extends BaseState implements IState<V>
}

@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

return this.rawError ? (isErrorObject(this.rawError) ? this.rawError.message : this.rawError) : undefined
return normalizeError(this.rawError)
}

@computed get error() {
Expand Down Expand Up @@ -108,7 +108,7 @@ export abstract class ValidatableState<V> extends BaseState implements IState<V>
action('end-validation', () => {
this.validation = undefined
this._validateStatus = ValidateStatus.Validated
this.setError(isValid(result) ? undefined : result)
this.setError(normalizeRawError(result))
})()
}

Expand Down
6 changes: 3 additions & 3 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ export type ValidationResult =
| null
| undefined
| false
| ErrorObject
| ValidationErrorObject

/** Return value of validator. */
export type ValidatorReturned =
Expand All @@ -19,9 +19,9 @@ export type Validation<TValue> = {
returned: ValidatorReturned // result of applying validators
}

export type ErrorObject = { message: string }
export type ValidationErrorObject = { message: string }
export type ValidationError = string | undefined
export type ValidationRawError = ErrorObject | ValidationError
export type ValidationRawError = ValidationErrorObject | ValidationError

export type ValidateResultWithError = { hasError: true, rawError: ValidationRawError, error: NonNullable<ValidationError> }
export type ValidateResultWithValue<T> = { hasError: false, value: T }
Expand Down
49 changes: 45 additions & 4 deletions src/utils.spec.ts
Original file line number Diff line number Diff line change
@@ -1,23 +1,24 @@
import { observable } from 'mobx'
import { asyncResultsAnd, isValid, isArrayLike, isErrorObject } from './utils'
import { asyncResultsAnd, isValidationPassed, isArrayLike, isErrorObject, normalizeRawError, normalizeError } from './utils'
import { delayValue as delay } from './testUtils'
import { ValidationErrorObject } from './types'

describe('asyncResultsAnd', () => {
it('should work well with empty results', async () => {
const result = await asyncResultsAnd([])
expect(isValid(result)).toBe(true)
expect(isValidationPassed(result)).toBe(true)
})

it('should work well with all-passed results', async () => {
const result = await asyncResultsAnd([delay(null)])
expect(isValid(result)).toBe(true)
expect(isValidationPassed(result)).toBe(true)

await asyncResultsAnd([
delay(null, 30),
delay(undefined, 10),
delay(false, 20)
])
expect(isValid(result)).toBe(true)
expect(isValidationPassed(result)).toBe(true)
})

it('should work well with unpassed results', async () => {
Expand Down Expand Up @@ -99,5 +100,45 @@ describe('isErrorObject', () => {
expect(isErrorObject({message: 'msg'})).toBe(true)
expect(isErrorObject({message: 'msg', extra: 'ext' })).toBe(true)
expect(isErrorObject(new Error('error msg'))).toBe(true)

class Foo implements ValidationErrorObject { message = 'mewo' }
expect(isErrorObject(new Foo())).toBe(true)

class Bar extends Foo {}
expect(isErrorObject(new Bar())).toBe(true)
})
})

describe('normalizeValidationResult', () => {
it('normalizeRawError should work well', () => {
expect(normalizeRawError(false)).toBe(undefined)
expect(normalizeRawError(null)).toBe(undefined)
expect(normalizeRawError(undefined)).toBe(undefined)
expect(normalizeRawError('')).toBe(undefined)
expect(normalizeRawError('foo')).toBe('foo')
expect(normalizeRawError({ message: 'mewo' })).toEqual({ message: 'mewo' })

class Foo implements ValidationErrorObject { message = 'mewo' }
const foo = new Foo()
expect(normalizeRawError(foo)).toBe(foo)

class Bar extends Foo {}
const bar = new Bar()
expect(normalizeRawError(bar)).toBe(bar)
})

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.

这是符合预期的吗

expect(normalizeError('foo')).toBe('foo')
expect(normalizeError({ message: 'mewo' })).toBe('mewo')

class Foo implements ValidationErrorObject { message = 'mewo' }
const foo = new Foo()
expect(normalizeError(foo)).toBe('mewo')

class Bar extends Foo {}
const bar = new Bar()
expect(normalizeError(bar)).toBe('mewo')
})
})
33 changes: 26 additions & 7 deletions src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,36 @@
import { isObservableArray, IObservableArray } from 'mobx'
import { Validator, ValidationResult, ValidatorReturned, ErrorObject } from './types'
import { Validator, ValidationResult, ValidatorReturned, ValidationError, ValidationErrorObject, ValidationRawError } from './types'

export function isErrorObject(err: any): err is ErrorObject {
export function normalizeRawError(err: ValidationResult): ValidationRawError {
if (isErrorObject(err)) {
return err
}

if (err === false || err === '' || err === null) {
// TODO: print an alert?
return undefined
}

return err
}

export function normalizeError(rawError: ValidationRawError): ValidationError {
if (isErrorObject(rawError)) {
return rawError.message
}
return rawError
}

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 !== ''

}

export function isPromiseLike(arg: any): arg is Promise<any> {
return arg != null && typeof arg === 'object' && typeof arg.then === 'function'
}

/** 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),而不是是否存在一份用于展示的错误文案(尽管这俩在结果上很接近)

return normalizeRawError(result) === undefined
}

export function asyncResultsAnd(asyncResults: Array<Promise<ValidationResult>>): ValidatorReturned {
Expand All @@ -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”

if (!isValid(result)) {
if (!isValidationPassed(result)) {
resolve(result)
return
}
Expand Down Expand Up @@ -56,7 +75,7 @@ export function applyValidators<TValue>(value: TValue, validators: Validator<TVa
}

// 任一不通过,则不通过
if (!isValid(returned)) {
if (!isValidationPassed(returned)) {
return returned
}
}
Expand Down