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

feat: destroy, unmount #198

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: destroy, unmount #198

wants to merge 1 commit into from

Conversation

pyongchen
Copy link

When the animation-mounted component is destroyed, the corresponding variables such as parents, mutations, resize, and timers are not cleared, which will cause memory leaks. Therefore, add a destroy method to autoAnimate, which is called when the component is destroyed. Memory leak related information is as follows:
企业微信截图_17100756826683
企业微信截图_1710075805294
Memory keeps rising as page routing switches

Copy link

vercel bot commented Mar 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
auto-animate ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 10, 2024 1:09pm

Copy link
Member

@justin-schroeder justin-schroeder left a comment

Choose a reason for hiding this comment

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

Hey!

Thanks for looking into this. Currently this pr assumes that there is only 1 auto animate instance but the module is sharing the same resize observer and mutation observer for all auto animate instances on the page, so we need to do a bit more scoping. Heading in the right direction though!

el,
setInterval(() => lowPriority(updatePos.bind(null, el)), 2000)
)
intervalIds.push(setInterval(() => lowPriority(updatePos.bind(null, el)), 2000));
Copy link
Member

Choose a reason for hiding this comment

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

These intervals are not scoped to the current element. This means if any unmount is called all the intervals for any animations on the page will be cleared. We should continue to use the intervals WeakSet.

Comment on lines +841 to +843
parents.delete(this.parent);
mutations?.disconnect();
resize?.disconnect();
Copy link
Member

Choose a reason for hiding this comment

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

We don’t want to disconnect the mutation observer or the resize observer. These are global instantiations (1 per load). Destroying them would destroy all other observers on the page. parents is the only non-weak set/map here so we should keep that one.

Comment on lines +844 to +845
intervalIds.forEach(id => clearInterval(id));
intervalIds = [];
Copy link
Member

Choose a reason for hiding this comment

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

These need to be scoped to the current parent using the intervals weakset.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants