-
Notifications
You must be signed in to change notification settings - Fork 8
fix: 基础 shape destroy 逻辑 #343
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: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -185,17 +185,20 @@ function createElement(parent: VNode, element: JSX.Element): VNode | VNode[] | n | |
| }); | ||
| } | ||
|
|
||
| function destroyElement(vNode: VNode | VNode[] | null) { | ||
| function destroyElement(vNode: VNode | VNode[] | null, nextVNode?: VNode) { | ||
| Children.map(vNode, (node: VNode | null) => { | ||
| if (!node) return; | ||
| const { component, children } = node; | ||
| const { component, children, shape, tag } = node; | ||
| if (component) { | ||
| component.willUnmount(); | ||
| destroyElement(children); | ||
| component.didUnmount(); | ||
| component.destroy(); | ||
| } else { | ||
| } else if (children) { | ||
| destroyElement(children); | ||
| } else if (tag === Shape && shape) { | ||
| // 更新模式时销毁上个节点 | ||
| nextVNode && shape.destroy(); | ||
| } | ||
| }); | ||
| } | ||
|
|
@@ -214,7 +217,7 @@ function updateElement(parent: VNode, nextElement: JSX.Element, lastElement: VNo | |
| } | ||
|
|
||
| const nextVNode = createVNode(parent, nextElement as VNode); | ||
| destroyElement(lastElement); | ||
| destroyElement(lastElement, nextVNode); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| return nextVNode; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,4 +42,209 @@ describe('vnode 更新', () => { | |
|
|
||
| expect(ref.current._vNode.props.update).toBe(true); | ||
| }); | ||
|
|
||
| it('基础图形更新', async () => { | ||
| const context = createContext(); | ||
| const { props } = ( | ||
| <Canvas context={context}> | ||
| <rect | ||
| style={{ | ||
| width: 36, | ||
| height: 36, | ||
| fill: 'red', | ||
| }} | ||
| /> | ||
| </Canvas> | ||
| ); | ||
|
|
||
| const canvas = new Canvas(props); | ||
| await canvas.render(); | ||
|
|
||
| const { props: nextProps } = ( | ||
| <Canvas context={context}> | ||
| <circle | ||
| style={{ | ||
| cx: 80, | ||
| cy: 30, | ||
| r: 20, | ||
| lineWidth: 2, | ||
| stroke: '#e45649', | ||
| fill: 'blue', | ||
| }} | ||
| /> | ||
| </Canvas> | ||
| ); | ||
| await delay(500); | ||
| await canvas.update(nextProps); | ||
|
|
||
| await delay(200); | ||
| expect(context).toMatchImageSnapshot(); | ||
| }); | ||
|
|
||
| it('基础图形销毁带离场动画', async () => { | ||
| const context = createContext(); | ||
| const { props } = ( | ||
| <Canvas context={context}> | ||
| <rect | ||
| style={{ | ||
| width: 36, | ||
| height: 36, | ||
| fill: 'red', | ||
| }} | ||
| animation={{ | ||
| leave: { | ||
| easing: 'ease-out', | ||
| duration: 500, | ||
| property: ['width', 'height'], | ||
| start: { | ||
| width: 36, | ||
| height: 36, | ||
| }, | ||
| end: { | ||
| width: 0, | ||
| height: 0, | ||
| }, | ||
| }, | ||
| }} | ||
| /> | ||
| </Canvas> | ||
| ); | ||
|
|
||
| const canvas = new Canvas(props); | ||
| await canvas.render(); | ||
|
|
||
| await delay(200); | ||
| const { props: nextProps } = ( | ||
| <Canvas context={context}> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 增加一个无离场动画的销毁 |
||
| </Canvas> | ||
| ); | ||
| await canvas.update(nextProps); | ||
| await delay(600); | ||
| expect(context).toMatchImageSnapshot(); | ||
| }); | ||
|
|
||
| it('基础图形更新带离场动画', async () => { | ||
| const context = createContext(); | ||
| const { props } = ( | ||
| <Canvas context={context}> | ||
| <rect | ||
| style={{ | ||
| width: 36, | ||
| height: 36, | ||
| fill: 'red', | ||
| }} | ||
| animation={{ | ||
| leave: { | ||
| easing: 'ease-out', | ||
| duration: 500, | ||
| property: ['width', 'height'], | ||
| start: { | ||
| width: 36, | ||
| height: 36, | ||
| }, | ||
| end: { | ||
| width: 0, | ||
| height: 0, | ||
| }, | ||
| }, | ||
| }} | ||
| /> | ||
| </Canvas> | ||
| ); | ||
|
|
||
| const canvas = new Canvas(props); | ||
| await canvas.render(); | ||
|
|
||
| await delay(200); | ||
| const { props: nextProps } = ( | ||
| <Canvas context={context}> | ||
| <circle | ||
| style={{ | ||
| cx: 80, | ||
| cy: 30, | ||
| r: 20, | ||
| lineWidth: 2, | ||
| stroke: '#e45649', | ||
| fill: 'blue', | ||
| }} | ||
| /> | ||
| </Canvas> | ||
| ); | ||
|
|
||
| // 无离场动画效果 | ||
| await canvas.update(nextProps); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 这个应该要有离场动画才对。
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
离场动画在销毁的时候才生效,从 A --> B 这种被当成了更新,会执行 B 的入场动画效果,和现有逻辑一致
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A->B 因为是完全不同的元素,所以走的不是更新吧,我感觉应该是销毁(离场)-> 入场 。
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 而且没有A的离场但会执行B的入场?不匹配吧
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 现在的逻辑是执行 B 的入场动画,从 A 到 B 的一个渐变过渡效果,如果等 A 的离场执行结束,再执行 B 的入场,过渡效果就没有了,和现在已有的整体效果完全不一样了
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 现在的逻辑不就是这份pr要改的bug吗。。。
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 你放的gif没看懂,是想说是哪个单测的效果呢
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
「而且没有A的离场但会执行B的入场?不匹配吧」现在所有的逻辑都是以 B 入场为准,从 A 到 B 会有个过渡效果,这个 pr 只是修复 bug ,不是增加 feature 啊,不变更已有逻辑
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 你要不本地跑下测试用例或者手动调整代码试下?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 群里直接沟通吧 |
||
| await delay(600); | ||
| expect(context).toMatchImageSnapshot(); | ||
| }); | ||
|
|
||
| it('基础图形更新带入场动画', async () => { | ||
| const context = createContext(); | ||
| const { props } = ( | ||
| <Canvas context={context}> | ||
| <circle | ||
| style={{ | ||
| cx: 30, | ||
| cy: 30, | ||
| r: 20, | ||
| lineWidth: 2, | ||
| stroke: '#e45649', | ||
| fill: 'blue', | ||
| }} | ||
| animation={{ | ||
| appear: { | ||
| easing: 'ease-in', | ||
| duration: 300, | ||
| property: ['r'], | ||
| start: { | ||
| r: 0, | ||
| }, | ||
| end: { | ||
| r: 20, | ||
| }, | ||
| }, | ||
| }} | ||
| /> | ||
| </Canvas> | ||
| ); | ||
|
|
||
| await delay(200); | ||
| const canvas = new Canvas(props); | ||
| await canvas.render(); | ||
|
|
||
| await delay(500); | ||
| const { props: nextProps } = ( | ||
| <Canvas context={context}> | ||
| <rect | ||
| style={{ | ||
| x: 80, | ||
| y: 30, | ||
| width: 36, | ||
| height: 36, | ||
| fill: 'red', | ||
| }} | ||
| animation={{ | ||
| update: { | ||
| easing: 'ease-out', | ||
| duration: 500, | ||
| property: ['width', 'height'], | ||
| start: { | ||
| width: 0, | ||
| height: 0, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 这个单测和你加的逻辑没关系 |
||
| }, | ||
| end: { | ||
| width: 36, | ||
| height: 36, | ||
| }, | ||
| }, | ||
| }} | ||
| /> | ||
| </Canvas> | ||
| ); | ||
|
|
||
| // 执行入场动画 | ||
| await canvas.update(nextProps); | ||
| await delay(600); | ||
| expect(context).toMatchImageSnapshot(); | ||
| }); | ||
|
|
||
| }); | ||
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.
当前的
destroyElement实现虽然解决了更新时基础图形的销毁问题(当提供了nextVNode时),但未能正确处理其他销毁场景,这可能会导致 bug 和内存泄漏。具体来说:
destroyElement的调用不会传入nextVNode。在这种情况下,nextVNode && shape.destroy()的判断会为 false,导致图形不会被销毁。destroyElement(children)。这个调用同样没有传递nextVNode,因此该组件下的所有基础图形子节点都不会被销毁。nextVNode这个参数及其相关检查似乎是不必要的,并且是导致这些问题的根源。无论是在更新还是删除场景下,只要destroyElement被调用,其对应的 shape 都应该被销毁。建议简化该函数,移除对
nextVNode的依赖,使销毁逻辑在所有情况下都健壮和正确。这个修改也需要更新在
updateElement中对destroyElement的调用(在第 220 行)。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.
我也感觉改这里怪怪的,ai写的挺有道理的
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.
直接 destroy 会导致
A --> null这种情况离场动画失效,A --> null的情况vnode会直接变为null,在 vnode tree 中会被去掉并销毁,目前版本的代码逻辑运行没问题