-
Notifications
You must be signed in to change notification settings - Fork 2k
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(vue): add unmount hook to unmount application #2849
Conversation
🦋 Changeset detectedLatest commit: ea3510f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@linghaoSu is attempting to deploy a commit to a Personal Account owned by @umijs on Vercel. @umijs first needs to authorize it. |
@@ -134,6 +149,10 @@ export const MicroApp = defineComponent({ | |||
); | |||
}); | |||
|
|||
onMounted(() => { |
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.
fixed
999867c
to
573e604
Compare
if (microApp) { | ||
microApp._unmounting = true; | ||
|
||
unmountMicroApp(microApp).catch((err: Error) => { |
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.
@kuitos 卸载这一块要不要 await 下,不等上一个应用A卸载完就加载应用B并跳转路由。有时候会匹配到A应用的404路由
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.
你是指通过同一个组件切 name 的方式切换子应用的场景的吗?
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.
是的, 改变name跟entry ,再push路由。
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.
那需要存一下前一个的 unmountPromise
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.
这个标志位好像作用也不大,按理来说应该是等上一个子应用完全卸载完再进行 路由跳转
@linghaoSu 漏了,切换应用是清除 error 信息 |
if (microApp) { | ||
microApp._unmounting = true; | ||
|
||
unmountMicroApp(microApp).catch((err: Error) => { |
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.
你是指通过同一个组件切 name 的方式切换子应用的场景的吗?
}, | ||
setup(props, { slots }) { | ||
const originProps = props; | ||
const { name, wrapperClassName, className, ...propsFromParams } = toRefs(originProps); | ||
const { name, wrapperClassName, className, appProps, ...propsFromParams } = toRefs(originProps); |
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.
有了 appProps 就不需要读 propsFromParams 了吧
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.
removed
|
||
microAppRef.value = undefined; | ||
} | ||
unmount(); |
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.
这里为啥要调 unmount ?
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.
利用 name 切换子应用时,先将前一个子应用 unmount
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.
这里应该可以去掉, 我看 最新的 mountMicroApp 已经有对上一个应用进行卸载的操作。 然后最好判断下 name 是否存在再调用mountMicroApp。
相关pr
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.
最新的 pr 应该没有在 mountMicroApp 进行卸载,只是做了如果 上一个 应用正在卸载,使用 await 等待卸载完毕再执行 mount 操作
@@ -118,14 +129,18 @@ export const MicroApp = defineComponent({ | |||
); | |||
|
|||
watch( | |||
reactivePropsFromParams, | |||
[reactivePropsFromParams, appProps], | |||
() => { | |||
updateMicroApp({ | |||
getMicroApp: () => microAppRef.value, |
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.
这里为啥要传一个 getter 而不是直接把 microApp 引用传过去? @bravepg
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.
这里为啥要传一个 getter 而不是直接把 microApp 引用传过去? @bravepg
确实,直接传值更简单,我改下
@linghaoSu 评论的问题再看一下 |
…ide props to sub-application
ok, 本周会处理一下 |
c981a2a
to
7ab7ffb
Compare
7ab7ffb
to
6fa797d
Compare
microAppProps: { | ||
...omitSharedProps(originProps), | ||
...appProps.value, | ||
}, | ||
}); | ||
}, | ||
{ |
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.
这里不应该不用深度监听,甚至 ref 都不用, shallowRef 就好。因为可能传递各种实例,或者组件啥的下去没必要 observe 一次。
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.
react 的版本现在就是 deepCompare 的
}, [useDeepCompare(omitSharedProps(componentProps))]); |
vue 里有更合理的方案实现类似的效果吗?
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.
不使用深度监听的话,以目前暴露的接口来说,是没有主动更新 vue 组件里的子应用的能力的,是否需要把子应用实例或者是更新方法暴露给使用者
此外,目前 vue 里如果使用 shallowRef 或者仅仅浅层 watch props 中的 appProps ,这种方式仅能检测到引用的变化,对于 appProps 内部的变化是无法检测到的,可能不太符合预期?
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.
一般来说 react 要改变 componentProps 里面的状态一般也是 {...componentProps} 这样吧,如果是这样 vue 这边可以用 shallowRef 然后 lodash.isEqual 比较新旧值来判断是否要更新 micro。 我是感觉 deep watch appProps 虽然简单但是有点耗性能。
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.
跟react版本保持一致吧,还是深度监听吧
packages/qiankun/src/version.ts
Outdated
@@ -1 +1 @@ | |||
export { version } from '../package.json'; | |||
export const version = '3.0.0-rc.19'; |
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.
已移除
a11df10
to
ea3510f
Compare
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.
LGTM
Checklist
npm test
passesDescription of change
related issue: #2846