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

optimize clearedFieldMetaCache #128

Merged
merged 3 commits into from
Dec 23, 2017
Merged

Conversation

HunDunDM
Copy link
Contributor

resetFields will reset clearedFieldMetaCache now.
The time to get clearedFieldMetaCache is getFieldProps now (saveRef before).

@HunDunDM
Copy link
Contributor Author

seem the code in saveRef can't be deleted. I'm thinking about the reason.......

@HunDunDM
Copy link
Contributor Author

In the original code, if use clearedFieldMetaCache in saveRef, will not call forceUpdate. So it will not rerender.

@HunDunDM
Copy link
Contributor Author

learn a lot because of this PR...

@afc163
Copy link
Member

afc163 commented Dec 14, 2017

Can you try to add a test case for this?

@HunDunDM
Copy link
Contributor Author

Let me try. I haven't written the unit test of jest before.

@HunDunDM
Copy link
Contributor Author

sad to find resetFields is still a problem.

add test case;
@HunDunDM
Copy link
Contributor Author

update resetFields and add two test cases.
The test cases is not passed with the previous code.

this.fieldsStore.setFields({
[name]: this.clearedFieldMetaCache[name].field,
});
this.fieldsStore.setFieldMeta(name, this.clearedFieldMetaCache[name].meta);
Copy link
Member

Choose a reason for hiding this comment

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

@afc163 这个实现是一直有问题的:

form/src/createBaseForm.js

Lines 309 to 314 in 7452074

if (this.clearedFieldMetaCache[name]) {
this.fieldsStore.setFields({
[name]: this.clearedFieldMetaCache[name].field,
});
this.fieldsStore.setFieldMeta(name, this.clearedFieldMetaCache[name].meta);
}

如果需要切换的两个组件,值的类型不一样,比如一个是 string 一个试 string[],这样切换会报错的,所以还是无法解决 #117 要解决的问题。

Copy link
Member

Choose a reason for hiding this comment

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

类型不一致就像给一个 string[] 去 setFieldValue 一个 string 类型的值一样。本来就不合理,不在要解决的范围内。

@benjycui
Copy link
Member

先 pending,等讨论清楚了再做决定。

@HunDunDM
Copy link
Contributor Author

HunDunDM commented Dec 15, 2017

首先,reset fields这部分的修改应该是没有疑问的。

然后,现在因为saveRef中读取cache之后不进行forceUpdate,导致有间隔的remount会在下一次render时发生闪烁。这个情况在Form和Modal同时使用的时候,极其容易触发。因此我认为有必要先防止这种情况的发生。@benjycui 提到的场景,由于当前版本定义field时并不指定类型,所以唯一的解决方案就是此处继续采用1.x版本的方案。

最后,关于解决这个问题的方案。

  • 1.x版本中,一个组件remount会清空value和error,只要在文档中说明这个情况的话,其实也算是一个解决方案。
  • 我PR中这个版本,则是组件unmount之后会暂时隐藏,然后在组件remount或者不绑定组件单独使用getFieldDecorator/Props,会将隐藏的部分重新暴露。(在官网demo中不定项表单例子中,有单独使用getFieldDecorator不绑定组件的场景)。这个方案还有一个需要讨论的点,就是field被暂时隐藏的这个时间段,setFields/getFields系列函数能否使用,如果可以使用,那么这些函数均需要recoverClearedField。如果不能,则需要在文档中说明,这种情况,需要先getFieldDecorator。目前来看,应该是所有方案里面,兼顾的最好的方案。
  • 在使用当前getFieldDecorator的逻辑的基础上,另一个不容易出现歧义的方案,是永远不要去自动隐藏field,让用户通过option.hidden来自动隐藏。但是这个方案在不定项Form中,又存在无法自动删除rerender后消失的fields,且使用起来极为不便,不如上一个方案。
  • 还有一个方案,就是将field的定义和getFieldDecorator/Props函数分开。但实际应用中,一个field的定义可能与另一个field的value有关,这个方案不够灵活。

@HunDunDM
Copy link
Contributor Author

Shall I repeat a PR with only resetFields?

@afc163
Copy link
Member

afc163 commented Dec 18, 2017

我看看。

expect(form.getFieldValue('input2')).toBe(undefined);
wrapper.setProps({ mode: true });
expect(wrapper.find('#text1').getDOMNode().value).toBe('123');
expect(wrapper.find('#text2').getDOMNode().value).toBe('456');
Copy link
Member

Choose a reason for hiding this comment

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

这个用例目前是有点奇怪的,会导致用户从 unmount 的控件切换回来时,会预填之前填过的值。很难讲应不应该是预期行为。

1

Copy link
Contributor Author

@HunDunDM HunDunDM Dec 18, 2017

Choose a reason for hiding this comment

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

如你所说,两者都不能确定是用户的预期。因此必然需要选择其一,并在文档中说明。

  • 1.x版本,切换会隐式reset。
  • 我PR的版本,预填之前填过的值。

我之所以PR会这么改,并且加上这个测试样例,是因为我觉得看目前写的代码,是想达到后者的效果。

但在当前的2.x ~ 2.1.6版本中,两者都不是,而是怎么看都是个bug,绝对不可能会是用户的预期。
我可以改成只修改resetFields的版本,但实际上没什么意义。
因为如果最后你们要采用方案一,则实际上我这个PR完全没用,相关代码均要删除,回到1.x。如果你们要采用方案二,就必然要解决下面这个bug,那就是整个merge。我还是希望能一次性搞定这个问题。
参考issue#127这个重现
步骤:

  • 在input1输入123,在input2输入456
  • 点击hidden,再点击visable。(此时,所有组件还看起来均为空)
  • 在任一组件随意输入字符,另一个组件闪烁并显示老数据。

当前版本,这个bug在hidden之后,无论是否点击reset均发生。只改resetFields则只能保证hidden后点击reset,再visable后不会闪烁

最后,提供一个不涉及Modal,也就是不会reset依然会发生的场景:Form中一项是选择添加的类型,根据这个类型,会展现不同的剩余item。

@afc163
Copy link
Member

afc163 commented Dec 18, 2017

先改成只有 resetFields 的 PR 吧,另一个问题需要提供具体的问题再看看。

然后,现在因为saveRef中读取cache之后不进行forceUpdate,导致有间隔的remount会在下一次render时发生闪烁。这个情况在Form和Modal同时使用的时候,极其容易触发

@HunDunDM
Copy link
Contributor Author

HunDunDM commented Dec 18, 2017

先改成只有 resetFields 的 PR 吧,另一个问题需要提供具体的问题再看看。

除了上面提到的重现

官网demo1 中 situation 3、 官网demo2
均存在一模一样的问题。

@afc163
Copy link
Member

afc163 commented Dec 18, 2017

👌,我晚点再看看 modal 这个问题。

@HunDunDM
Copy link
Contributor Author

f0q5-3ttyk17tos5a-4o

@afc163
Copy link
Member

afc163 commented Dec 23, 2017

销毁组件的时候还是直接干掉对应的表单值比较合理。对于 https://codesandbox.io/s/o4nwr2x7z 我找到一个更合理的解决方案,我先合并了这个再发一个 PR。

@afc163 afc163 merged commit db23161 into react-component:master Dec 23, 2017
afc163 added a commit that referenced this pull request Dec 23, 2017
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