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] Artalk not working in Vue Project #658

Closed
Mister-Hope opened this issue Dec 15, 2023 · 24 comments
Closed

[Bug] Artalk not working in Vue Project #658

Mister-Hope opened this issue Dec 15, 2023 · 24 comments
Labels
pr-welcome Welcome to pull request question Further information is requested

Comments

@Mister-Hope
Copy link
Contributor

Mister-Hope commented Dec 15, 2023

Refs that you may need to follow:

Some important notes:

image


Note: Artalk is never working fine in any of my Vue Project from the beginning to present.

As VuePress core team member and probably a senior Vue coder, I have already checked and refactored my code many times.

Artalk component will be full re-rendered as we use different keys in different pages here

Artalk instance is destroyed here as described in docs.

A new Artalk instance is initialized once the dom is rendered/rerendered here

@qwqcode qwqcode added pr-welcome Welcome to pull request question Further information is requested labels Dec 15, 2023
@qwqcode
Copy link
Member

qwqcode commented Dec 15, 2023

你好,之前和您进行讨论已在 #426 #373 等 PR、issues 进行积极应对和修复,感谢您的反馈与建议!

但这并不是像你所说的:

”我跟他已经有一次没有结果的讨论了。“

之后没有收到你的其他反馈,并且我这边已测试无问题,则视为问题已经修复。

VitePress 的案例目前已经修改为了 NPM 包引入,

而并不是像您所说的:

”我坚持认为 Artalk 那个包就是有问题的。实际上我好奇过为什么作者在 Artalk 使用 VitePress 建文档时候,不使用仓库内编译好的 Artalk 包,而非得要用 script 引入 CDN,我强烈怀疑他自己也跑不起来就用了 script 脚本堆这了。“

您可以参考以下代码:

https://github.com/ArtalkJS/Artalk/blob/f8c33003a88fdb01e163c650c7b469b8d343d66c/docs/.vitepress/theme/Artalk.vue

再次感谢您的反馈!

我知道您是 VuePress core team member and a senior Vue coder,感谢您对开源社区的贡献!

但根据你的描述,我暂时无法确定是什么问题,欢迎进一步的讨论。


Hello,

Thank you for the proactive handling and fixes on PRs and issues such as #426 and #373, following our previous discussions. Your feedback and suggestions are greatly appreciated.

However, it's not as you mentioned: "I had one discussion with him that led nowhere."

After that, I didn't receive further feedback from you, and after testing on my end, I confirmed there were no issues, so I considered the problem fixed.

The use case for VitePress has now been modified to include the NPM package.

Contrary to what you mentioned: "I still believe that the Artalk package has issues. I'm actually curious why the author, while documenting Artalk using VitePress, didn't utilize the precompiled Artalk package in the repository and instead chose to use script tags to import CDN. I strongly suspect that they couldn't get it running themselves, so they resorted to piling script tags."

You can refer to the following code: https://github.com/ArtalkJS/Artalk/blob/f8c33003a88fdb01e163c650c7b469b8d343d66c/docs/.vitepress/theme/Artalk.vue

Thank you once again for your feedback!

I understand that you're a VuePress core team member and a senior Vue coder. Thank you for your contributions to the open-source community!

However, based on your description, I'm temporarily unable to determine the exact issue. I welcome further discussion.

@Mister-Hope
Copy link
Contributor Author

  1. 我有在其他帖子里跟你讨论过这个问题,但是因为时间过于久远我找不到了,当时你明确归结于 Vue 框架的问题

  2. 在第一个讨论里,我汇报了在切换页面时异常的内存增长,对于你写的新组件代码,如果用户在没有评论的主页和有评论的页面之间重复切换,那么你实际在没有销毁 artalk 任何监听器的情况下单纯将 artalk dom 移除并加载新 dom 初始化 artalk。根据标准的开发规范,我假定你提供的 ArtalkInsance.destory 是有效的,那你应该在 unBeforeUnmounted 周期调用,以释放 artalk 可能保留资源。所以我不承认你写的是一个标准的 artalk Vue 组件。

  3. 关于内存泄漏问题,我当时汇报以及 23年 6 月我试图重新拾起 artalk 兼容的时候,我仍然观察到持续的内存增长。作为普通 Artalk 用户,我实在没有时间审查你的代码检查每一个监听器的调用,以及任何函数中可能存在的闭包陷阱导致对象无法 GC 的情况。这意味着我需要使用源代码编译并对内存进行分析,而且我需要对源代码有比较高的熟悉。而且我也不认为普通用户应该明确指出问题以及修复才可以提出内存泄露的问题。

  4. 我可以确定,在同一个 dom 上反复进行 Artalk.init 和 artalkInstance.destroy 会出现报错。对于一个标准的 lib,它应该提供销毁方法并真实移除了在此 dom 上附加的所有内容。

我最近在忙我的毕业论文,不过如果你有需要,我可以提供上述相关内容的复现。

@qwqcode
Copy link
Member

qwqcode commented Dec 15, 2023

哈哈哈哈,Vue 框架能有什么问题?😁 我从来没有归结于 Vue 框架的问题,你这有点奇怪哈。

你是不是偏题了?你之前 issue 我已经修了。#368 #373

回到本 issue 关注的问题,目前我的 VitePress 和其他 Vue 项目都能正常使用 Artalk,我对于你的 issue 比较迷惑。

请你有空的话,麻烦使用最新代码,提供上述相关内容的复现,并完成汇报,以帮助定位问题,谢谢!


你指的框架是你的 VuePress?nextTick 行为异常问题,你不是修了吗?

image

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Dec 15, 2023

另外我重新回顾了一下这里的内容。根据你的 code,你的 artalk 很奇怪,似乎从来没有考虑过多个实例的问题。

如果你的下方代码有效:

import Artalk from 'artalk';

Artalk.init({/* xxx */}))

Artalk.update({/* xxx */}))

Artalk.destroy()

那我的确不大理解 Artalk 的设计。我只能理解为 Artalk 从不提供工厂函数,并将内部数据和状态在包的上下文(而不是创建的实例)内进行存储,至少我一直认为的是这样的东西:

import Artalk from 'artalk';

const artalkInstance1 = Artalk.init({/* xxx */})
const artalkInstance2 = Artalk.init({/* xxx */})

artalkInstance1.update({/* xxx */}))
artalkInstance2.update({/* xxx */}))

// artalk element is removed and all data and listeners GCed
artalkInstance1.destroy()
artalkInstance2.destory()

这样我就不是难理解为什么我在同一个元素上 init destoryinit 无效了。我认为这是设计上的问题。

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Dec 15, 2023

你指的框架是你的 VuePress?nextTick 行为异常问题,你不是修了吗?

那个 discussion 算是比较早的东西了。你可以暂时忽略 discussion 的内容。

算了你忽略上面的东西吧,我总结一下。


我的核心问题可以分成以下几个部分:

  1. 你的 Artalk 没有任何监听器么?如果目前你上方贴出来的组件被频繁的 mount/unmounted,而你组件里从来不调用 destroy 方法,不会存在任何闭包陷阱/未销毁的监听器/其他内存泄露问题?

  2. Artalk 的状态数据在哪进行存储?包的上下文还是实例,如果是实例,我没看到 Artalk 入口 class 有任何的 magic code 来自动操作最新实例。

import Artalk from 'artalk';

Artalk.init({/* xxx */}))

Artalk.update({/* xxx */}))

Artalk.destroy()

如果是包上下文,artalk 设计上没有考虑过多实例问题么?如果我的确涉及到用 keep-alive 来缓存多个组件实例,而这些组件实例上有不同的 artalk 实例,我该怎么办?

  1. 针对2的问题以及这种奇怪的调用方式,如果说挪出 Vue 的范围,如果我需要:
  • el1 上初始化 artalk
  • 在 el2 上初始化另一个 artalk
  • 移除 el1
  • 移除 el2
  • 在 el1 上初始化 artalk
  • 移除 el1 并确认这时 artalk 已经 fine GCed

我该如何调用 API?

@qwqcode
Copy link
Member

qwqcode commented Dec 15, 2023

😁 你之前指出会存在内存泄露的问题,我采纳你的观点并改成单例模式以节省内存(以前存在不必要的 DOM 拷贝问题),这些都已经修复。#373 (comment)

为什么要没必要 copy 的数据需要创建多个对象副本?为你提供了 update、reload 等 API 你从来就不看文档吗?

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Dec 15, 2023

所以说这就是我说的跟你交流很容易没有下文的原因,我的问题你是一个都不正面回答....

为什么要没必要 copy 的数据需要创建多个对象副本?为你提供了 update、reload 等 API 你从来就不看文档吗?

如果你觉得在一个页面同时在不同的el上挂载不同的 artalk 是奇葩的请求,

如果我的确涉及到用 keep-alive 来缓存多个组件实例,而这些组件实例上有不同的 artalk 实例,我该怎么办?

这个东西总是合理的吧?

@qwqcode
Copy link
Member

qwqcode commented Dec 15, 2023

你发的 issue 有正面描述问题吗?我不明白。

请提供测试用例。

再次麻烦您使用最新代码,提供上述相关内容的复现,以帮助定位问题。

根据你的描述,我暂时无法确定是什么问题,谢谢。

@Mister-Hope
Copy link
Contributor Author

我从来没说你的 reload 啥的不工作或者设计的不好。

image
我的核心观点一致就两个,对于 artalk:

  1. init() => destroy() => init() => destroy() 就报错
  2. 这种做法会出现异常的内存增长

@qwqcode
Copy link
Member

qwqcode commented Dec 15, 2023

我从来没说你的 reload 啥的不工作或者设计的不好。

image 我的核心观点一致就两个,对于 artalk:

  1. init() => destroy() => init() => destroy() 就报错
  2. 这种做法会出现异常的内存增长

这个早已修复了,我已回复您,请您参考:https://github.com/ArtalkJS/Artalk/blob/f8c33003a88fdb01e163c650c7b469b8d343d66c/docs/.vitepress/theme/Artalk.vue

如果有问题,请提供测试用例,麻烦您使用最新代码,提供上述相关内容的复现,以帮助定位问题。

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Dec 15, 2023

你的代码的 destroy 在哪里?还是 Artalk 不需要任何额外工作完成 GC?那你设计 destory 的目的是什么

image

@qwqcode
Copy link
Member

qwqcode commented Dec 15, 2023

destroy() 请参考代码:https://github.com/ArtalkJS/Artalk/blob/f8c33003a88fdb01e163c650c7b469b8d343d66c/ui/packages/artalk/src/artalk.ts#L83-L88C4

触发 destroy 事件,以及 Artalk.instance.$root.remove() 等操作

@qwqcode
Copy link
Member

qwqcode commented Dec 15, 2023

我的用例里面没有调用 destroy 的需求,我只需要调用 update 即可实现功能,不会发生内存泄露。请根据实际应用需求调用 destroy,反复创建和销毁实例同样不是好的实践。

@ArtalkJS ArtalkJS locked as too heated and limited conversation to collaborators Dec 15, 2023
@ArtalkJS ArtalkJS unlocked this conversation Dec 15, 2023
@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Dec 15, 2023

#373 (comment)

这个更改我当时就没有去追踪了,我查代码查到这里了,这是一个负面而且不好的更改。

技术上说,这也是我第三遍贴这个东西了:

如果我的确涉及到用 keep-alive 来缓存多个组件实例,而这些组件实例上有不同的 artalk 实例,我该怎么办?

对于一个 SPA,留有多个 Artalk 实例是现实的,你的更改反而引发了新的问题。

@Mister-Hope
Copy link
Contributor Author

Mister-Hope commented Dec 15, 2023

正确的做法就是:

// 每次初始化会初始化一个实例
const instance = Artalk.init({})

// 调用 instance 的方法来操作相应的实例
instance.setDarkmode()
instance.update()
instance.reload()

// 移除整个 dom 并销毁所有 artalk 相关的监听器
instance.destroy()

这要求你的所有文件都不得在文件内保存状态,所有的状态管理在 instance 上。

因为我不用 artalk 当时我提出 issue 之后 就没太细看你究竟是怎么处理的,以为你修了就完了。

我建议你借鉴一下隔壁我写的 Waline https://waline.js.org/cookbook/reactivity.html

响应式要做的东西有很多,因为网络请求相关的更新通常是异步的,所以一些完美的做法包括:

  • 新的更新自动取消正在进行得前一个操作
  • 初始化和更新操作应该进行额外检查以避免在异步过程中用户已经销毁导致的 监听器未移除 / 操作不存在的 Dom 之类的问题。

Artalk 改成单例之后在我眼里看来问题可大了去了。你不能总理想化的认为,artalk 只需要在 SPA 有一个实例并且认为这个 dom 一直 onScreen。

对于我的项目,用户可以使用多个 CommentService 也允许缓存它们,同时用户可以自由控制各个页面评论的启用情况,所以我需要考虑多实例、以及 Artalk 销毁得问题。而且我的确在 real experience 里,多次看到最新的 artalk 也常常抛出错误。

举一个例子,如果用户行为导致了一次 artalk 的更新或者初始化,但是用户马上切换到其他无评论页面,destroy 调用发生在 init/reload 完成前,我在我的项目里会看到错误。你确定你的代码考虑了这些情况吗?

@qwqcode
Copy link
Member

qwqcode commented Dec 15, 2023

很高兴你能给出改进的建议,这非常有价值,由于学期末有很多DDL时间有限,我以后会认真考虑并修改!谢谢 Thanks♪(・ω・)ノ

@Mister-Hope
Copy link
Contributor Author

注意上方有更新

举一个例子,如果用户行为导致了一次 artalk 的更新或者初始化,但是用户马上切换到其他无评论页面,destroy 调用发生在 init/reload 完成前,我在我的项目里会看到错误。你确定你的代码考虑了这些情况吗?

我不是来找茬的,VuePress 各插件涉及到的第三方包,只有 artalk 在不停地报错。🫠 作为拥有数个 1k star+ 项目的开发者,我认为我对一个情景算不算常规应该还是有判断力的,我觉得我也没搞什么 edge case。

希望 artalk v3 能够解决上述问题。

@Mister-Hope
Copy link
Contributor Author

很高兴你能给出改进的建议,这非常有价值,由于学期末有很多DDL时间有限,我以后会认真考虑并修改!谢谢 Thanks♪(・ω・)ノ

我也忙,我周末预答辩,这些问题可以留给 v3,我觉得你可能需要一次完整的重构,因为你非常有可能在某些文件里存在局部状态。

我的个人建议是你可以使用 solid.js 这种运行时随代码量线性增加的库。引入一个现有的响应式的框架可以很好的解决这类问题。原生开发很容易在细节上产生问题导致局部状态污染、内存泄漏这些。

@qwqcode
Copy link
Member

qwqcode commented Dec 15, 2023

局部状态、全局变量确实是不好的实践。我一直在思考怎么重构,因为 Artalk 不是依赖 Vue 框架开发的组件,响应式框架集成原生 JS 的组件如何才能很好的兼容,如何提供好的开发者体验,这些问题一直困扰着我,由于时间和能力有限,有些问题暂时没有好的解决方案,好在你的建议让我有了点头绪!

(。・_・。)ノI’m sorry~ 之前我没有理解是什么问题,可能有什么误解,抱歉

再次感谢你的建议和对开源社区的贡献 ❤️

@qwqcode
Copy link
Member

qwqcode commented Dec 15, 2023

祝你答辩顺利!🍀

@qwqcode
Copy link
Member

qwqcode commented Dec 17, 2023

v2.7.0 (2023-12-17) 新版已发布

该版前端允许创建多个实例。相关文档已补充:前端 API置入框架

已在 Vue + VueRouter、Vitepress (SSG) 应用测试 initdestroy 的 API,应该已解决实际问题。

感谢你的反馈和帮助!如遇其他问题,欢迎随时反馈!❤️

@Mister-Hope
Copy link
Contributor Author

如果可以,其实可以提供一个 vue/react 组件导出,算是一个 feature request

@qwqcode
Copy link
Member

qwqcode commented Dec 17, 2023

如果可以,其实可以提供一个 vue/react 组件导出,算是一个 feature request

OK!以后慢慢试着糊一个出来 🤣

跟踪:#670

@Mister-Hope
Copy link
Contributor Author

The 2.7.1 fixed my problem

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-welcome Welcome to pull request question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants