-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Add onDataChanged method to handle data changes in the plugin #16244
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
base: dev
Are you sure you want to change the base?
Conversation
c2ba4dc to
f387e77
Compare
|
可以 review 了 |
|
|
||
| export const reloadPlugin = async (app: App, data: { upsertPlugins: string[], removePlugins: string[] }) => { | ||
| data.removePlugins.forEach((item) => { | ||
| export const reloadPlugin = async (app: App, data: { upsertCodePlugins?: string[], upsertDataPlugins?: string[], removePlugins?: string[] } = {}) => { |
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.
upsertCodePlugins 这个为什么要加 Code?
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.
插件代码有变化就需要重新加载插件
|
|
||
| export const reloadPlugin = async (app: App, data: { upsertPlugins: string[], removePlugins: string[] }) => { | ||
| data.removePlugins.forEach((item) => { | ||
| export const reloadPlugin = async (app: App, data: { upsertCodePlugins?: string[], upsertDataPlugins?: string[], removePlugins?: string[] } = {}) => { |
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.
upsertDataPlugins 会有对应的 removeData 么?这个参数是 Data,应该不用加 plugins 后缀了。
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.
插件删除配置文件也算在 upsertDataPlugins 内
| } | ||
| }); | ||
| } | ||
| reloadPlugins.push(pluginName); |
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.
需要去重
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.
|
|
看了你的回复,发现这并不是处理 data change 的。抱歉,没注意看前面的 #15444 (comment) 。如果要处理禁用状态下卸载插件的话,可以加一个阶段为 uninstalled,即卸载成功后的回调。你看这样可以么? |
|
前面那个 issue 之所以需要在没有启用插件的情况下卸载插件,是因为把 onunload 方法用来处理插件配置同步了,现在增加一个新的 onDataChanged 方法就可以使用正常的 onunload 方法在前端卸载插件了。 我感觉没有什么办法能在不启用插件的情况下执行 uninstall。 |
|
这个是卸载的问题,和 data change 没有关系。这样开发者可能无法理解,我也不太理解。一开始我就误解为插接数据变化的同步。 |
|
确实是插件存储的数据的变化,就是 this.data |
|
正常情况下 onunload() 应该移除插件在前端添加的所有组件,但之前的逻辑是存储数据变更会先执行 onunload() 再执行 onload(),同步一下插件配置就会把组件全部移除的话交互太奇怪了,所以我就把完整的 onunload() 逻辑移到 uninstall() 里去了,然后就会导致一个问题,如果用户先禁用插件再卸载插件的话就只会执行 onunload(),一部分组件还留在界面上。 增加一个单独的 onDataChanged() 方法的话,我就可以把 uninstall() 里的逻辑移回 onunload(),把 onDataChanged() 用来处理配置同步,onunload() 用来完整移除组件,uninstall() 只额外执行一步 removeData(),这样就基本没有在禁用插件时卸载插件执行 uninstall() 的需求了。 |
|
实际上这个 PR 单纯就是用来处理数据同步的,不看之前那个 issue 也完全没有影响 |
|
如果不看前一个 issue,把这个做为数据更改的话,它又和 onunload,uninstall 这些生命周期混杂在一起。还是没搞懂这个的用途和价值。 |
|
目前数据同步之后插件会执行 onunload() 和 onload(),增加一个 onDataChanged() 就不需要执行 onunload() 和 onload() 了 |
|
但是 changeDate 不应该和 onunload() onload() 搅合在一起 |
|
我没理解你的意思 |
|
onDataChanged 应该只是在数据有改动时回调他,不应该去 onunload 和 onload |
|
那我的代码有什么问题呢? |
|
如果要解决 #15444 (comment) 这个问题的话,不应该叫 dataChange;如果要添加 dataChange 的话,不应该和生命周期混在一起。 |
|
这个 PR 不能实现“在禁用状态卸载插件”,而是能让我的插件不再需要“在禁用状态卸载插件”这个需求 |
|
onDataChanged 不应该加到 reloadPlugins 中。在 data 变化的时候去执行 onunload,uninstall 等操作会导致生命周期乱套的。 |
|
plugin 只要执行了 onDataChanged() 就不会被加到 reloadPlugins 中 |
|
一般用户都不会添加这个事件。不论是否执行 onDataChanged 都不应该去额外的执行 onunload,uninstall 等操作。 |
|
没看懂什么叫“额外的执行 onunload,uninstall 等操作”。对于没有使用 onDataChanged() 的插件来说,逻辑跟以前相比没有发生变化。 |
给插件添加 onDataChanged 方法,用来替代同步之后插件先 onunload 再 onload,解决我在 #15444 (comment) 遇到的问题
基于前一个 PR 修改,需要先合并:#16243
需要测试: