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

refactor(components): [backTop] refactor #5371

Closed
wants to merge 1 commit into from

Conversation

buqiyuan
Copy link
Member

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?

  1. Describe the source of requirement.
  2. Resolve what problem.
  3. Related issue link.

API Realization (Optional if not new feature)

  1. Basic thought of solution and other optional proposal.
  2. List final API realization and usage sample.
  3. GIF or snapshot should be provided if includes UI/interactive modification.

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

  1. Does this PR affect user? Which part will be affected?
  2. What will say in changelog?
  3. Does this PR contains potential break change or other risk?

Changelog description (Optional if not new feature)

  1. English description
  2. Chinese description (optional)

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)

If this PR related with other PR or following info. You can type here.

export type BackTopProps = Partial<ExtractPropTypes<typeof backTopProps>>;

const BackTop = defineComponent({
name: 'ABackTop',
inheritAttrs: false,
props: backTopProps,
emits: ['click'],
emits: backTopEmits,
Copy link
Member

Choose a reason for hiding this comment

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

我们计划去掉 emits 声明,全都放入 props 中去声明

Copy link
Member Author

@buqiyuan buqiyuan Mar 20, 2022

Choose a reason for hiding this comment

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

🤔全部放props里面?这样不好吧,vue不同于react,react所有的属性事件回调children(插槽)都是通过props传递,但在vue中是将react的props拆分为了props + emits + slots相对独立的3部分,虽然在vue中也完全可以像react那样,将所有东西都通过props进行传递,但两者对props的处理还是略有不同的。我觉得咱们应该遵循vue的设计理念,propsemits应该是分开来的,自定义事件都应该定义在emits中,如果都定义在props中,在对antdv组件进行二次封装的时候,如果把antdv组件原有的props解构到封装的组件props上的话,再将当前组件的props透传给antdv组件的时候,我需要手动剔除一些属性,遇到的一些小问题,可以看下我这里留下的评论

Copy link
Member Author

@buqiyuan buqiyuan Mar 20, 2022

Choose a reason for hiding this comment

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

在之前issue中也能找到一些相关的问题,但是在setup-script语法中,即使通过defineEmits定义了,但这些事件仍会被合并成一个数组

Copy link
Member

Choose a reason for hiding this comment

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

我觉得合并数组的问题可以尝试给 vue 提PR,添加配置项
还有一点是定义在 emits 中有几个小问题:
1、无法获取用户是否传递了某些事件,有些组件会根据是否传递有不同的行为,vue 未来可能会支持
2、无法获取事件返回值,有些事件需要返回值,如 true、false、promise 等,过去我们单独处理了这块

如果想统一掉,props 是一个合适(唯一)的选择

PS:在 vue2中,props + emits + slots相对独立,在 vue3 中已经渐渐淡化这个概念了,而且 slots 未来有可能也会有定义在 props 中的语法糖:vuejs/rfcs#192 (comment)

Copy link
Member Author

@buqiyuan buqiyuan Mar 21, 2022

Choose a reason for hiding this comment

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

关于您提到的两点:
1、可以通过getCurrentInstance()?.vnode?.props拿到用户是否传递了某些事件(emit),这里的getCurrentInstance()?.vnode?.props其实就是完全等同于react的props了,也就是你现在定义在props里面的所有东西,这里vnode的props是包含了component的props + attrs的。
2、自定义事件(emit)我认为是不需要返回值的,因为父组件仅仅是监听了子组件内部触发的事件并接收了回调参数而已,需要返回值的通过props传递一个回调函数即可,自定义事件(emit)和回调函数(callback)应该是要区分开来的。

PS:在vue3中,以前vue2中适用的概念确实被淡化了不少,但我觉得slots被定义在props这个可能性不大,而且之前vue与ts存在的类型系统断层问题,现在通过工具链的支持已经变得很好了,即是说vue与ts相性不好的问题通过工具链的桥接一样可以做到react + ts的体验。
image
image

Copy link
Member

Choose a reason for hiding this comment

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

getCurrentInstance()?.vnode?.props 算是一个非文档 API(黑科技),随时都有可能会改动,antdv1.x 就是用了很多黑科技,导致升级很困难,我们应该尽可能避免这类使用,虽然现在还有一些”黑科技“,但一直在慢慢偿还债务。

对于组件库来说,统一规范定义在props,可以减少很多工作量。
而对于普通用户来说,是无感知的。
对于二开用户或二次封装用户,这都不是事。

Copy link
Member Author

Choose a reason for hiding this comment

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

对于非文档中公开的API(黑科技)的使用确实需要考量,行!那我明白了。然后大概看了下目录源码,感觉还可以精简很多

};
export type BackTopEmits = typeof backTopEmits;

export type BackTopInstance = InstanceType<typeof BackTop>;
Copy link
Member

Choose a reason for hiding this comment

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

这个主要用来做什么用呢

Copy link
Member Author

Choose a reason for hiding this comment

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

这些用到的时候自然有用,例如:通过ref调用组件实例方法的时候

Copy link
Member

Choose a reason for hiding this comment

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

这个我们只需要暴露组件内 expose 出去的方法就好了,其他方法都是不安全的,不应该暴露给用户。
如果非要用,在用户层面自己 InstanceType 好了

Copy link
Member Author

Choose a reason for hiding this comment

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

OK

@buqiyuan buqiyuan closed this Mar 21, 2022
@github-actions
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 Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants