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

ownError & hasOwnError for IState #70

Merged
merged 3 commits into from
Mar 1, 2022
Merged

ownError & hasOwnError for IState #70

merged 3 commits into from
Mar 1, 2022

Conversation

nighca
Copy link
Collaborator

@nighca nighca commented Feb 24, 2022

fix #71 , see details there.

@nighca nighca changed the title WIP Own error WIP ownError & hasOwnError for IState Feb 24, 2022
@nighca nighca changed the title WIP ownError & hasOwnError for IState ownError & hasOwnError for IState Feb 24, 2022
@nighca nighca mentioned this pull request Feb 24, 2022
Merged
13 tasks
@nighca nighca force-pushed the own-error branch 2 times, most recently from d171f2a to 4948d35 Compare February 24, 2022 14:49
@nighca nighca linked an issue Feb 25, 2022 that may be closed by this pull request
this.validation = undefined

override reset() {
super.reset()
Copy link
Collaborator Author

@nighca nighca Feb 25, 2022

Choose a reason for hiding this comment

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

这里把多个 XxxState 中公共的对校验状态信息的重置逻辑抽到 ValidatableState reset() 中了

Copy link
Contributor

@Luncher Luncher left a comment

Choose a reason for hiding this comment

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

🍎

@@ -70,7 +74,7 @@ interface IV2StateFromV3<T extends v3.IState<V>, V> extends v2.ComposibleValidat
$: T
}

class Downgrader<T extends v3.IState<V>, V> extends BaseState implements IV2StateFromV3<T, V> {
class Downgrader<T extends v3.IState<V>, V> extends Disposable implements IV2StateFromV3<T, V> {
Copy link
Contributor

Choose a reason for hiding this comment

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

这是为了避免暴露: ownError 吗?

Copy link
Collaborator Author

@nighca nighca Feb 28, 2022

Choose a reason for hiding this comment

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

嗯,ownError 也是跟 v2 state 无关的东西,继承 BaseState 变得不划算了

Copy link
Collaborator

Choose a reason for hiding this comment

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

v2 不是也有 ownError 吗?

Copy link
Collaborator Author

@nighca nighca Mar 1, 2022

Choose a reason for hiding this comment

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

v2 的 ownError 只在 class FormState 上,而不在 IV2StateFromV3(对应 v2.ComposibleValidatable)上

@nighca
Copy link
Collaborator Author

nighca commented Mar 1, 2022

35d1bce 给 override 的 action(reset)加了下 @override(虽然没有导致问题),参考 https://mobx.js.org/subclassing.html

@@ -39,6 +39,10 @@ export interface IState<V = unknown> {
error: Error
/** If the state contains error. */
hasError: boolean
/** The state's own error info, 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.

states of children?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

预期这个概念就叫 child state,即,组成一个 state 的子 state;这个概念会贯穿所有的文档还有注释,比如这里 https://nighca.github.io/formstate-x/concepts/state#composability

@@ -147,6 +156,13 @@ export abstract class ValidatableState<V> extends BaseState implements IState<V>
return this
}

@action reset() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIP: 类似这种应该 abstract 吗?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

abstract method 吗?如果是 abstract 它就不能有实现了,这里我是希望去抽出公共的实现的

Copy link
Collaborator

Choose a reason for hiding this comment

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

ts 的 abstract 是可以有实现的吧?

@@ -61,25 +48,21 @@ abstract class AbstractFormState<T, V> extends ValidatableState<V> implements IS
}

/** If reference of child states has been touched. */
@observable protected _dirty = false
@observable protected ownDirty = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

其实没看懂… 比如:
为什么都有 ownError 但不一定都有 ownDirty
而且 own 的注释是 child

Copy link
Collaborator Author

@nighca nighca Mar 1, 2022

Choose a reason for hiding this comment

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

为什么都有 ownError 但不一定都有 ownDirty

如果把 own dirty 放到 interface IState 上,也是没有逻辑上的问题的,只是不需要:state 的使用者确实需要区分 own error 跟 error 以分别使用,但 state 的使用者只关心 dirty 而不会关心 own dirty(目前来看)

这里 AbstractFormState 存在字段 ownDirty 只是为了实现其 dirty,而不是为了给使用方

而且 own 的注释是 child

参考 #70 (comment) 这里,child state(s) 是一个概念,即,FormState / ArrayFormState 中的那些子 state;所以这里注释是说

If reference of child states has been touched

即,“对于子 state 的引用是否被更改”,区别于“子 state 的内部值是否被更改”

这里的注释是可能可以优化的,有建议么?

@@ -70,7 +74,7 @@ interface IV2StateFromV3<T extends v3.IState<V>, V> extends v2.ComposibleValidat
$: T
}

class Downgrader<T extends v3.IState<V>, V> extends BaseState implements IV2StateFromV3<T, V> {
class Downgrader<T extends v3.IState<V>, V> extends Disposable implements IV2StateFromV3<T, V> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

v2 不是也有 ownError 吗?

if (isV2FormState(stateV2)) {
return stateV2.ownError
}
return stateV2.error
Copy link
Collaborator

Choose a reason for hiding this comment

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

为什么不 throwNotSupported

Copy link
Collaborator Author

@nighca nighca Mar 1, 2022

Choose a reason for hiding this comment

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

严格一点应该是:

// branch 1
if (isV2FormState(stateV2)) {
  return stateV2.ownError
}
// branch 2
if (isV2FieldState(stateV2)) {
  return stateV2.error
}
// branch 3, not sure
throwNotSupported()

而 own error 需要跟 error 进行区别,只在于那些由 child state 组合而成的 state。所以对于 branch 3 可以走跟 branch 2 一样的逻辑,还是比较有信心的

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

如当面对的,先 throw,如果实践中遇到问题再去看是不是优化

46cc32d

@@ -99,9 +106,11 @@ class Downgrader<T extends v3.IState<V>, V> extends BaseState implements IV2Stat
return getV2ValidateStatus(this.stateV3)
}

// for BaseState
@computed get validateStatus() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

所以这是 bug fix 吗?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

不是 bug fix,原本为了复用 BaseState,导致 Downgrader 比预期的 IV2State 多实现了一个字段 validateStatus;现在把这个没有对外的字段干掉

this._error = undefined
this.validation = undefined

override reset() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

类似这种,是特地去掉这个 @action 的吗?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

不是,如 #70 (comment) 这里所述,预期应该加上 @override 以确保其保持 action 的行为

@override override get ownError() {
if (this.disabled) return undefined
if (this._error) return this._error
return this.$.ownError
Copy link
Collaborator

@lzfee0227 lzfee0227 Mar 1, 2022

Choose a reason for hiding this comment

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

有点忘了,为什么其他值要 sync 或 delay
但是 errorownError 能实时同步呢?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

有可能 error & ownError 也走一下 sync with debounce 会好一点,不过这个对使用者影响比较微妙,所以之前没有考虑这边细节

不过这个跟这个 PR 的目的关系不大,我拿个 PR 单独处理一下这边 error sync 的细节吧

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.

lgtm

@nighca nighca merged commit c7ff3bc into qiniu:v3.x Mar 1, 2022
nighca added a commit to nighca/formstate-x that referenced this pull request Mar 4, 2022
* ownError & hasOwnError for IState

* @OverRide for action

* throw for ownError of non-recognized v2 state
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.

ownError for IState
3 participants