-
Notifications
You must be signed in to change notification settings - Fork 9
[RMBWEB-2780] Support for structured validation results #87
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
Changes from 1 commit
83cd99a
8d68591
f466115
db9f109
227f425
d84c44a
90e4499
78f35e5
b2f88ba
e5afe2a
3e45f64
511d66f
b8fb666
9065ddf
5d56725
2c76ca8
a8cb353
031f8a1
ba4b45e
dbba8f5
a5ec69d
82e505b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,13 @@ interface IV3StateFromV2<T extends v2.ComposibleValidatable<unknown, V>, V> exte | |
| $: 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>>> | ||
Luncher marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| class Upgrader<T extends v2.ComposibleValidatable<unknown, V>, V> extends BaseState implements IV3StateFromV2<T, V> { | ||
| constructor(private stateV2: T) { | ||
| super() | ||
|
|
@@ -27,6 +34,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() { | ||
|
|
@@ -42,7 +50,7 @@ class Upgrader<T extends v2.ComposibleValidatable<unknown, V>, V> extends BaseSt | |
| setForV2(this.stateV2, value) | ||
| } | ||
| reset() { this.stateV2.reset() } | ||
| withValidator(...validators: Array<v3.Validator<V>>) { | ||
| withValidator(...validators: Array<LegacyValidator<V>>) { | ||
Luncher marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| if ( | ||
| isV2FieldState(this.stateV2) | ||
| || isV2FormState(this.stateV2) | ||
|
|
@@ -64,7 +72,7 @@ class Upgrader<T extends v2.ComposibleValidatable<unknown, V>, V> extends BaseSt | |
| } | ||
| } | ||
|
|
||
| /** Convets [email protected] state to [email protected] state */ | ||
| /** 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) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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, isErrorObject } from './utils' | ||
|
|
||
| const defaultDelay = 200 // ms | ||
|
|
||
|
|
@@ -52,7 +52,7 @@ export class DebouncedState<S extends IState<V>, V = ValueOf<S>> extends Validat | |
|
|
||
| @override override get ownError() { | ||
|
||
| if (this.disabled) return undefined | ||
| if (this._error) return this._error | ||
| if (this._error) return isErrorObject(this._error) ? this._error.message : this._error | ||
| return this.original.ownError | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -373,4 +373,31 @@ 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'}) | ||
|
||
| ) | ||
|
|
||
| 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', rawError: { message: 'error-object-msg' }}) | ||
| }) | ||
|
|
||
| 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', rawError: { message: 'error-object-msg' }}) | ||
| }) | ||
| }) | ||
| }) | ||
| 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, isValid, isPromiseLike, isErrorObject } from './utils' | ||||||
|
|
||||||
| /** Extraction for some basic features of State */ | ||||||
| export abstract class BaseState extends Disposable implements Pick< | ||||||
|
|
@@ -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 | ||||||
|
||||||
| return this.disabled ? undefined : this._error | |
| return this.disabled ? undefined : this._error |
Outdated
There was a problem hiding this comment.
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) 这个逻辑都是成立的
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
不太明确 BaseState 的定位,我看 BaseState 上一些属性,跟 ValidatableState 关系也挺大的
There was a problem hiding this comment.
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 信息
There was a problem hiding this comment.
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 都成立的
There was a problem hiding this comment.
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 成立),那走这个逻辑会对实现更方便?
There was a problem hiding this comment.
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 成立),那走这个逻辑会对实现更方便?
我之前在这里就说了啊..,现在要把实现改回去吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
我之前在这里就说了啊..
那边我没看懂你的意思..
现在要把实现改回去吗?
我倾向改回去,如果确实 hasError = !!error 这个逻辑还成立,而且基于这个逻辑确实更方便的话
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ba4b45e
done
Luncher marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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() | ||
|
||
| expect(state.hasError).toBe(true) | ||
| expect(state.error).toBe('empty') | ||
| expect(state.ownError).toBe('empty') | ||
| expect(state.rawError).toEqual({ message: 'empty' }) | ||
| }) | ||
| }) | ||
|
|
||
| interface Host { | ||
|
|
@@ -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' }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -4,6 +4,7 @@ export type ValidationResult = | |||
| | null | ||||
| | undefined | ||||
| | false | ||||
| | ErrorObject | ||||
|
|
||||
| /** Return value of validator. */ | ||||
| export type ValidatorReturned = | ||||
|
|
@@ -18,9 +19,11 @@ export type Validation<TValue> = { | |||
| returned: ValidatorReturned // result of applying validators | ||||
| } | ||||
|
|
||||
| export type ErrorObject = { message: string } | ||||
Luncher marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||
| export type ValidationError = string | undefined | ||||
| export type ValidationRawError = ErrorObject | ValidationError | ||||
|
||||
| this.setError(isValid(result) ? undefined : result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
从概念上讲,
ValidationResult跟ValidationResp这种比较接近
ValidationResp 是啥?
之前的实现好像有意的区分,
Result跟Error的区别
是的,Result 跟 Error 是两个概念;不过 RawError 跟 Result 可以做成一个概念?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Result跟Error是两个概念
当然可以不区分这两个概念我觉得会更好,只是因为从方便 validator 实现的角度来说,需要允许它 return boolean | string | undefined | null,而从方便 error 消费的角度来说会希望它的取值是 string | undefined;所以这边拆了两个概念来达到这个目的
如果我们评估觉得,对 validator 来说只允许它 return string | undefined 也还算方便,且有比较好的兼容历史(validator)代码的方法,那么我会倾向于把 Result 跟 Error 也合成一个概念的
There was a problem hiding this comment.
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 实现的角度来说我觉得这个还是留着比较好。
There was a problem hiding this comment.
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 不重要”这一结论,那么我也会倾向把 Result 跟 Error 合成一个概念
这边是两个事情:
Result&Error概念合并Result&RawError概念合并
1 我们肯定不会现在去做的,也不用现在去评估;之所以上边提到 1 是因为 1 跟 2 的动力是一样的:概念越少理解起来越简单
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
建议不要放在
error&ownError上说明;不需要用rawError的人就不需要接触ErrorObject之类的概念是最好There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这里是从这边 state 同步过来的
意思是,
error&ownError实现上包含了ErrorObejct, 但是这边不说吗?..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
嗯,我感觉这里不说应该问题不大,毕竟类型上也会反映它只会是
string | undefined;当他真的想要通过 validator 返回ErrorObejct的时候,那时他能找到对应的文档就好