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

[Bug] destroy function for artalk to avoid memory leak in SPA #373

Closed
Mister-Hope opened this issue Feb 3, 2023 · 5 comments · Fixed by #426
Closed

[Bug] destroy function for artalk to avoid memory leak in SPA #373

Mister-Hope opened this issue Feb 3, 2023 · 5 comments · Fixed by #426
Labels
bug Something isn't working

Comments

@Mister-Hope
Copy link
Contributor

Mister-Hope commented Feb 3, 2023

Artalk needs an destroy method to remove all event listerner and callback functions to prevent memory increasing.

Also, the destroy method should remove extra dom besides cleaning the original object.

E.g.: div.atk-layer-wrap it inserts during initialization.

image

@qwqcode
Copy link
Member

qwqcode commented Feb 3, 2023

Discussed in #367 (reply in thread)

You have reminded me that there may indeed be a memory leak, but I haven't actually verified it.

In my case, I use the el.remove() function to free the dom that artalk is working on, and I use const type declarations to avoid this "shadowing" effect.

if (artalkEl) artalkEl.remove()

const artalk = new (window as any).Artalk(conf);

This is really a point to ponder, and I'm not quite sure if there are potential places where it's not CGed. (Although I try to only create listeners associated with one dom of artalk rather than lots of event listeners on the window object)

Anyway, repeatedly creating new is not a good choice, especially in spa, and I also prefer to abandon the previous initialisation way (and instantiate the object only once).

Ensuring that it can be effectively CGed requires some finesse, which also creates difficulty in developing programs that don't leak memory. It is easy to make this mistake.

Concerning the scope of the impact: in a spa page, the program's data is saved continuously, but in a static page (such as a hugo blog), the original data is lost every time the page is refreshed, so it is not affected.

@qwqcode qwqcode added the bug Something isn't working label Feb 3, 2023
@qwqcode
Copy link
Member

qwqcode commented Feb 3, 2023

Thanks for the issue, it's really helpful!

@qwqcode qwqcode changed the title [Bug] Destroy method for artalk [Bug] destroy function for artalk to avoid memory leak Feb 3, 2023
@qwqcode qwqcode changed the title [Bug] destroy function for artalk to avoid memory leak [Bug] destroy function for artalk to avoid memory leak in SPA Feb 3, 2023
@qwqcode qwqcode moved this to Todo in Artalk 2024 Feb 3, 2023
@inkss
Copy link
Contributor

inkss commented Feb 5, 2023

为什么不尝试去掉 id 选择器部分?移除 #ctx-${ctx.cid}id="ctx-${ctx.cid}"

export function GetLayerWrap(ctx: Context): { $wrap: HTMLElement, $mask: HTMLElement } {
let $wrap = document.querySelector<HTMLElement>(`.atk-layer-wrap#ctx-${ctx.cid}`)
if (!$wrap) {
$wrap = Utils.createElement(
`<div class="atk-layer-wrap" id="ctx-${ctx.cid}" style="display: none;"><div class="atk-layer-mask"></div></div>`
)

@qwqcode
Copy link
Member

qwqcode commented Feb 5, 2023

为什么不尝试去掉 id 选择器部分?移除 #ctx-${ctx.cid}id="ctx-${ctx.cid}"

export function GetLayerWrap(ctx: Context): { $wrap: HTMLElement, $mask: HTMLElement } {
let $wrap = document.querySelector<HTMLElement>(`.atk-layer-wrap#ctx-${ctx.cid}`)
if (!$wrap) {
$wrap = Utils.createElement(
`<div class="atk-layer-wrap" id="ctx-${ctx.cid}" style="display: none;"><div class="atk-layer-mask"></div></div>`
)

因为当初设计是想多个 Artalk 实例互不干扰,一页有多个互相隔离的独立 Artalk 实例,例如两个评论框,他们不会因为公用相同的 Layer Wrap 元素而造成干扰。

但这个功能似乎并没有什么用,既增加了代码复杂度,难以维护,又为前端部署带来问题,以后想直接删掉这个特性,只允许一个页面创建一个 Artalk 实例,类似单例模式的实现。

@qwqcode qwqcode moved this from Todo to In Progress in Artalk 2024 Mar 2, 2023
@qwqcode qwqcode linked a pull request Mar 6, 2023 that will close this issue
@qwqcode
Copy link
Member

qwqcode commented Mar 10, 2023

v2.5.0 (2023-03-10) 新版已发布

https://github.com/ArtalkJS/Artalk/releases

新版 API 文档:https://artalk.js.org/develop/fe-api.html

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants