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

fix: add array type to event cb props's declaration (#5410 #5414) #6138

Closed
wants to merge 2 commits into from

Conversation

merfais
Copy link

@merfais merfais commented Dec 5, 2022

related issue #5127 #5138

First of all, thank you for your contribution! 😄

New feature please send pull request to feature branch, and rest to master branch. Pull request will be merged after one of collaborators approve. Please makes sure that these form are filled before submitting your pull request, thank you!

[中文版模板 / Chinese template]

This is a ...

  • New feature
  • Bug fix
  • Site / document update
  • Component style update
  • TypeScript definition update
  • Refactoring
  • Code style optimization
  • Branch merge
  • Other (about what?)

What's the background?

fix issue #5410

API Realization (Optional if not new feature)

none

What's the effect? (Optional if not new feature)

none

Changelog description (Optional if not new feature)

  1. English description
  2. 修复绑定多个事件处理函数时,事件接口校验报类型错误waring的问题

Self Check before Merge

  • Doc is updated/provided or not needed
  • Demo is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Changelog is provided or not needed

Additional Plan? (Optional if not new feature)

none

@tangjinzhou
Copy link
Member

单纯的改这个治标不治本,调用事件的地方也有很多不支持数组
所以建议组件不支持,自己业务方去处理

@merfais
Copy link
Author

merfais commented Dec 13, 2022

单纯的改这个治标不治本,调用事件的地方也有很多不支持数组 所以建议组件不支持,自己业务方去处理

修正的是不给warning,并不是要处理数组类型的回调函数。这个warning并不是使用数组类型的事件绑定才会出现,而且业务中也不应该绑定数组类型的事件回调,如 onEvent="[eventCallback]"因为这不是react,vue绑定事件应该通过 @eventName="eventCallback"这种方式,尽管vue支持了onEvent="eventCallback"这种方式。

产生这个warning的原因,我在 issue #5414已经阐述,并给出了demo,我这里再贴一次,请你再次试图理解下这个原因,如果实在理解不了,就算了。很失望。


不应该把onUpdate:value作为props定义的接口,更不应该限定props.onUpdate:value只接受function,这不符合vue的范式()。

vue3本身支持了attrs的透传,props的合并与event事件合并策略是不一样的。antd-vue的代码却采用了react式的处理方式(事件也是props,即函数的参数,事件通过onXXX回调范式作为props传入子组件,事件的合并需要用户自己控制),但vue中event和props是不同的,是会做区分的,props与event的合并策略也是不一样的,props合并采用的覆盖策略, event采用的是合并到Array的策略。因此antd-vue粗暴的阉割这个规则是不合适的。

检索了部分antv的源码,也并未发现有直接使用 props.onXXX() 这种方式处理事件回调,如果我修改的文件中有使用这种方式调用的,可以comment指出,我可以提交commit取消这个文件的修改,使用这种调用,支持了array类型确实可能会出错,但这种调用本来就不应该使用,出错我更倾向于是antv设计失误导致。

在antd-vue的代码中并没有真实的使用了props.onUpdate:value的值,仅仅是做了校验(只接受function),否则如果传了Array类型的数据就直接报warning(vue本省能力,props校验失败给warning)。见上面的例子,并没有渲染异常,只在console中给了warning,而且行为也是符合预期的,子组件的onInputUpdate和父组件中的onParentUpdate函数都会执行,但这种在开发模式下的waring看着很烦(正式环境这些的waring会被去掉,这是vue自身的能力)。

demo

@merfais
Copy link
Author

merfais commented Dec 13, 2022

@tangjinzhou 望与其他maintainer共同探讨下这个pr,别轻易下结论,你自己也提交过一个类似的commit,并关闭了一个issue 5127不知道你是否还记得原因。

@tangjinzhou
Copy link
Member

第一,antd-vue粗暴的阉割这个规则 是客观存在的实时,不反驳
第二,更改的确是一种改进,但有很大的隐患,单测也并没有覆盖这部分测试
第三,如果不能补全这部分单测,和详细排查事件的调用方,粗暴的更改props,并不是一个好的选择,warning至少在开发阶段就给出了提示,避免了线上隐患
第四,之前的提交,是详细排查了调用方不会引入bug,才提交了这个 commit。当然我并不是说你没有详细排查,只是现在全量的替换,我们同样也需要一个个的去review排查调用方是否只需要更改props
第五,综合考虑,控制台很好的给出了 warning 提示用户不能使用数组,用户完全可以先行自己处理
第六,当然我们也欢迎你继续针对该问题提交 pr,不过建议你单个组件提交 pr,如有可能可以添加针对该问题的单元测试

第七,我们下个大版本也会考虑进去

@github-actions
Copy link

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the Stale label Feb 14, 2023
@github-actions github-actions bot closed this Feb 21, 2023
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants