Closed LittleBadBad closed 2 years ago
ostli
commented
2 years ago 赞👍
提两个改进建议哈：
1、sandbox和内存泄漏的，分成两个pr
2、代码做下封装
1)重复的拼接，抽出方法吧：
file: outRoot + 'lib/index.js',
file: outRoot + 'es/index.js',
output: [{file: outRoot + 'typings/index.d.ts', format: 'es'}],
...
2）getURL重复太多次，能否优化，封装下？
url: getURL(this.config.sandbox)('guildAnnounceURI'),
url: getURL(this.config.sandbox)('channelAnnounceURI'), ...
`
ostli
commented
2 years ago @LittleBadBad @clover14580 由于没有收到您的后续反馈，我们计划参考这次PR，由我们维护提交，并会备注来源。 如有异议请留言～ 感谢🌹
LittleBadBad
commented
2 years ago 抱歉最近比较忙没有及时回复，就您上次提出的问题1. 关于内存泄漏和sandbox分开提交的，我首先修复的内存泄露，后来修复的sandbox中也包含了对上一步修复的一部分完善，分开提交可能不太合适
张楚研 腾讯校园空间
联系我
------------------ 原始邮件 ------------------ 发件人: @.>; 发送时间: 2022年1月22日(星期六) 中午11:52 收件人: @.>; 抄送: @.>; @.>; 主题: Re: [tencent-connect/bot-node-sdk] 修复ws循环重复监听EVENT_WS事件导致内存泄露问题 (PR #39)
@LittleBadBad @clover14580 由于没有收到您的后续反馈，我们计划参考这次PR，由我们维护提交，并会备注来源，感谢🌹
— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.Message ID: @.***>
ostli
commented
2 years ago 抱歉最近比较忙没有及时回复，就您上次提出的问题1. 关于内存泄漏和sandbox分开提交的，我首先修复的内存泄露，后来修复的sandbox中也包含了对上一步修复的一部分完善，分开提交可能不太合适 2. 关于getURL封装的问题，之前的sandbox应用方案是出于使用编辑器批量修改方便考虑； 再者本次再次修复了一些问题 1. 对ws的重连机制进行修复，对WebsocketCloseReason新增resume字段，以校验本次断开重连是否需要携带上一次的session； 2. config中新增retry字段，并重新确定连接成功的标志，防止业务逻辑层面（非网络问题）的断连导致的无限循环重连问题 3. 关于getURL封装的问题，以给出封装方案，已在部分类中有修改，全部修改代码量较大，暂未完全更改 您这边看一下，是否需要我这边提新的pr 张楚研 腾讯校园空间 联系我 … ------------------ 原始邮件 ------------------ 发件人: @.>; 发送时间: 2022年1月22日(星期六) 中午11:52 收件人: @.>; 抄送: @.>; @.>; 主题: Re: [tencent-connect/bot-node-sdk] 修复ws循环重复监听EVENT_WS事件导致内存泄露问题 (PR #39) @LittleBadBad @clover14580 由于没有收到您的后续反馈，我们计划参考这次PR，由我们维护提交，并会备注来源，感谢🌹 — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.Message ID: @.***>
你好楚研 @LittleBadBad ，目前我刚提了一个新的pr #42 ，包括对sandbox的支持，以及一些细节优化，部分改动来自你的这个pr。
由于内部业务调整，目前比较急把内存泄漏和sandbox都加上，所以我这边先推进了。
如果你方便，可以在我那个pr基础上(已merged)，将性能优化这块补充上，你看OK不？我们计划24号前，把这件事情落地，辛苦哈～
LittleBadBad
commented
2 years ago 可以的，还是push到fix/xxx分支吗
张楚研 腾讯校园空间
联系我
------------------ 原始邮件 ------------------ 发件人: "tencent-connect/bot-node-sdk" @.>; 发送时间: 2022年1月22日(星期六) 下午5:33 @.>; @.**@.>; 主题: Re: [tencent-connect/bot-node-sdk] 修复ws循环重复监听EVENT_WS事件导致内存泄露问题 (PR #39)
抱歉最近比较忙没有及时回复，就您上次提出的问题1. 关于内存泄漏和sandbox分开提交的，我首先修复的内存泄露，后来修复的sandbox中也包含了对上一步修复的一部分完善，分开提交可能不太合适 2. 关于getURL封装的问题，之前的sandbox应用方案是出于使用编辑器批量修改方便考虑； 再者本次再次修复了一些问题 1. 对ws的重连机制进行修复，对WebsocketCloseReason新增resume字段，以校验本次断开重连是否需要携带上一次的session； 2. config中新增retry字段，并重新确定连接成功的标志，防止业务逻辑层面（非网络问题）的断连导致的无限循环重连问题 3. 关于getURL封装的问题，以给出封装方案，已在部分类中有修改，全部修改代码量较大，暂未完全更改 您这边看一下，是否需要我这边提新的pr 张楚研 腾讯校园空间 联系我 … ------------------ 原始邮件 ------------------ 发件人: @.>; 发送时间: 2022年1月22日(星期六) 中午11:52 收件人: @.>; 抄送: @.>; @.>; 主题: Re: [tencent-connect/bot-node-sdk] 修复ws循环重复监听EVENT_WS事件导致内存泄露问题 (PR #39) @LittleBadBad @clover14580 由于没有收到您的后续反馈，我们计划参考这次PR，由我们维护提交，并会备注来源，感谢🌹 — Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.Message ID: @.***>
你好楚研 @LittleBadBad ，目前我刚提了一个新的pr #42 ，包括对sandbox的支持，以及一些细节优化，部分细节优化改动来自你的这个pr。
由于内部业务调整，目前比较急把内存泄漏和sandbox都加上，所以我这边先推进了。
如果你方便，可以在我那个pr基础上，将泄漏这块补充上，你看OK不？
— Reply to this email directly, view it on GitHub, or unsubscribe. Triage notifications on the go with GitHub Mobile for iOS or Android. You are receiving this because you were mentioned.Message ID: @.***>
ostli
commented
2 years ago 嗯嗯 我的那个pr已经merged了，辛苦从main新拉分支：fix/xxx
断开连接时，client中会建立新的session，而event依然不变，每次建立新的session，event都会重新on一次EVENT_WS事件，ws中的EventEmitter并不具备识别重复事件的能力