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
Merged
Show file tree
Hide file tree
Changes from 7 commits
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
10 changes: 8 additions & 2 deletions dumi/docs/concepts/state/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ export interface IState<V> {
error: ValidationError
/** The state's own error info, regardless of child states. */
ownError: ValidationError
/** The state's own error info, includes ValidationErrorObject error type, regardless of child states. */
rawError: ValidationRawError
/** Append validator(s). */
withValidator(...validators: Array<Validator<V>>): this
/** Fire a validation behavior imperatively. */
Expand Down Expand Up @@ -67,11 +69,11 @@ Validation is the process of validating user input values.
Validation is important for cases like:

* When user inputs, we display error tips if validation not passed, so users see that and correct the input
* Before form submiiting, we check if all value is valid, so invalid requests to the server can be avoided
* Before form submitting, we check if all value is valid, so invalid requests to the server can be avoided

That's why validation should provide such features:

* It should run automatically, when users changed the value, or when some other data change influcend the value validity
* It should run automatically, when users changed the value, or when some other data change influenced the value validity
* It should produce details such as a meaningful message, so users can get friendly hint

With formstate-x, we define validators and append them to states with `withValidator`. formstate-x will do validation for us. Through `validateStatus` & `error`, we can access the validate status and result.
Expand All @@ -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 `ValidationErrorObject`. You can check details about them in issue [#82](https://github.com/qiniu/formstate-x/issues/82).

### Disable State

You may find that we defined method `disableWhen` to configure when a state should be disabled. It is useful in some specific cases. You can check details in section [Disable State](/guide/advanced#disable-state).
4 changes: 4 additions & 0 deletions dumi/docs/concepts/validator/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,10 @@ export type ValidationResult =
| null
| undefined
| false
| ValidationErrorObject

/** Object type validation result. */
export type ValidationErrorObject = { message: string }

/** Return value of validator. */
export type ValidatorReturned =
Expand Down
1 change: 1 addition & 0 deletions src/adapter/v2.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -705,6 +705,7 @@ describe('toV2', () => {
touched = false
ownError = undefined
error = undefined
rawError = undefined
activated = false
validateStatus = v3.ValidateStatus.NotValidated
async validate() {
Expand Down
22 changes: 20 additions & 2 deletions src/adapter/v2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ 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 (formstate-x@v2.x) state */
Expand All @@ -27,6 +28,7 @@ class Upgrader<T extends v2.ComposibleValidatable<unknown, V>, V> extends BaseSt
@computed get ownError() {
return getV3OwnError(this.stateV2)
}
@computed get rawError() { return this.ownError }
@computed get error() { return this.stateV2.error }
@computed get activated() { return this.stateV2._activated }
@computed get validateStatus() {
Expand All @@ -47,7 +49,7 @@ class Upgrader<T extends v2.ComposibleValidatable<unknown, V>, V> extends BaseSt
isV2FieldState(this.stateV2)
|| isV2FormState(this.stateV2)
) {
this.stateV2.validators(...validators)
this.stateV2.validators(...portV2Validators(...validators))
return this
}
throwNotSupported()
Expand All @@ -64,7 +66,23 @@ class Upgrader<T extends v2.ComposibleValidatable<unknown, V>, V> extends BaseSt
}
}

/** Convets formstate-x@v2.x state to formstate-x@v3.x state */
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 formstate-x@v2.x state to formstate-x@v3.x state */
export function fromV2<T extends v2.ComposibleValidatable<unknown, unknown>>(stateV2: T): IV3StateFromV2<T, T['value']> {
return new Upgrader(stateV2)
}
Expand Down
28 changes: 28 additions & 0 deletions src/debouncedState.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,20 @@ describe('DebouncedState validation', () => {
expect(state.error).toBe('empty')
expect(state.ownError).toBe('empty')
})

it('should work well with resolved error object', async () => {
const fooState = new FieldState('')
const formState = new FormState({ foo: fooState })
const state = new DebouncedState(formState, defaultDelay).withValidator(
() => ({ message: 'mock msg' })
)

await state.validate()
expect(state.hasError).toBe(true)
expect(state.ownError).toBe('mock msg')
expect(state.error).toBe('mock msg')
expect(state.rawError).toEqual({ message: 'mock msg' })
})
})

function createFieldState<T>(initialValue: T, delay = defaultDelay) {
Expand Down Expand Up @@ -607,4 +621,18 @@ describe('DebouncedFieldState validation', () => {
expect(validator).toBeCalled()
expect(state.validateStatus).toBe(ValidateStatus.Validated)
})

it('should work well with resolved error object', async () => {
const state = createFieldState(0).withValidator(
_ => ({ message: 'empty' })
)

state.validate()

await delay()
expect(state.hasError).toBe(true)
expect(state.error).toBe('empty')
expect(state.ownError).toBe('empty')
expect(state.rawError).toEqual({ message: 'empty' })
})
})
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 } 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 this._error
if (this._error) return normalizeError(this._error)
return this.original.ownError
}

Expand Down
39 changes: 39 additions & 0 deletions src/fieldState.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -373,4 +373,43 @@ describe('FieldState validation', () => {
assertType<string>(res.value)
}
})

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.

好多这种少了右空格的 😂

)

const res = await state.validate()
expect(state.hasError).toBe(true)
expect(state.error).toBe('error-object-msg')
expect(state.rawError).toEqual({ message: 'error-object-msg'})
expect(res).toEqual({ hasError: true, error: 'error-object-msg'})
})

it('should work well with sync resolved empty message', async () => {
const state = new FieldState('').withValidator(
_ => ({ message: ''})
)

const res = await state.validate()
expect(state.hasError).toBe(true)
expect(state.error).toBe('')
expect(state.rawError).toEqual({ message: ''})
expect(res).toEqual({ hasError: true, error: ''})
})

it('should work well with async resolved', async () => {
const state = new FieldState('').withValidator(
_ => null,
_ => delayValue({ message: 'error-object-msg' }, 100)
)

const res = await state.validate()
expect(state.hasError).toBe(true)
expect(state.error).toBe('error-object-msg')
expect(state.rawError).toEqual({ message: 'error-object-msg'})
expect(res).toEqual({ hasError: true, error: 'error-object-msg'})
})
})
})
53 changes: 53 additions & 0 deletions src/formState.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -607,6 +607,59 @@ describe('FormState (mode: object) validation', () => {
assertType<{ foo: string }>(res.value)
}
})

describe('should work well with resolved error object', () => {
it('should work well with mixed sync and async resolved error object', async () => {
const initialValue = { foo: '123', bar: '123' }
const state = new FormState({
foo: new FieldState(initialValue.foo),
bar: new FieldState(initialValue.bar)
}).withValidator(
({ foo, bar }) => delayValue(foo === bar && { message: 'same' }),
({ foo }) => foo === '' && { message: 'empty' }
)
state.validate()

await delay()
expect(state.hasError).toBe(true)
expect(state.error).toBe('same')
expect(state.ownError).toBe('same')
expect(state.rawError).toEqual({ message: 'same' })

state.$.foo.onChange('')

await delay()
expect(state.hasError).toBe(true)
expect(state.error).toBe('empty')
expect(state.ownError).toBe('empty')
expect(state.rawError).toEqual({ message: 'empty' })

state.$.foo.onChange('456')

await delay()
expect(state.hasError).toBe(false)
expect(state.error).toBeUndefined()
expect(state.rawError).toBeUndefined()
})

it('should work well with child states resolved error object', async () => {
const state = new FormState({
foo: new FieldState(''),
bar: new FieldState('').withValidator(
v => v === '' && { message: 'empty' }
)
})
state.validate()
await delay()
expect(state.hasError).toBe(true)
expect(state.error).toBe('empty')
expect(state.rawError).toBeUndefined()
expect(state.ownError).toBeUndefined()
expect(state.$.bar.hasError).toBe(true)
expect(state.$.bar.error).toBe('empty')
expect(state.$.bar.rawError).toEqual({ message: 'empty' })
})
})
})

describe('FormState (mode: array)', () => {
Expand Down
20 changes: 12 additions & 8 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 { ValidationError, IState, Validation, ValidateResult, ValidateStatus, Validator } from './types'
import { ValidationRawError, ValidationError, IState, Validation, ValidateResult, ValidateStatus, Validator } from './types'
import Disposable from './disposable'
import { applyValidators, isValid, isPromiseLike } 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 All @@ -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)的能力才是更合理的?

}

abstract ownError: ValidationError
Expand Down Expand Up @@ -56,10 +56,14 @@ export abstract class ValidatableState<V> extends BaseState implements IState<V>
/**
* The original error info of validation.
*/
@observable protected _error: ValidationError
@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

}

@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.disabled ? undefined : this._error
return normalizeError(this.rawError)
}

@computed get error() {
Expand All @@ -69,7 +73,7 @@ export abstract class ValidatableState<V> extends BaseState implements IState<V>
/**
* Set error info.
*/
@action setError(error: ValidationError) {
@action setError(error: ValidationRawError) {
this._error = error
}

Expand Down Expand Up @@ -104,13 +108,13 @@ 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))
})()
}

@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 的

? { hasError: true, error: this.error } as const
: { hasError: false, value: this.value } as const
)
Expand Down
27 changes: 27 additions & 0 deletions src/transformedState.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,19 @@ describe('TransformedState (for FieldState) validation', () => {
expect(state.ownError).toBe('non positive')
})

it('should work well with resolved error object', async () => {
const state = createNumState(0).withValidator(
_ => ({ message: 'empty' })
)

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()

expect(state.hasError).toBe(true)
expect(state.error).toBe('empty')
expect(state.ownError).toBe('empty')
expect(state.rawError).toEqual({ message: 'empty' })
})
})

interface Host {
Expand Down Expand Up @@ -738,4 +751,18 @@ describe('TransformedState (for FormState) validation', () => {
expect(state.hasOwnError).toBe(false)
expect(state.ownError).toBeUndefined()
})

it('should work well with resolved error object', async () => {
const state = createHostState('127.0.0.1').withValidator(
_ => ({ message: 'mock msg'})
)

state.validate()

await delay()
expect(state.hasError).toBe(true)
expect(state.error).toBe('mock msg')
expect(state.ownError).toBe('mock msg')
expect(state.rawError).toEqual({ message: 'mock msg' })
})
})
4 changes: 4 additions & 0 deletions src/transformedState.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ export class TransformedState<S extends IState<$V>, V, $V = ValueOf<S>> extends
return this.$.ownError
}

@computed get rawError() {
return this.$.rawError
}

@computed get error() {
return this.$.error
}
Expand Down
Loading