Skip to content

fix: connect eventBus block#2843

Closed
piLurk wants to merge 1 commit intomainfrom
fix/connection-event-block
Closed

fix: connect eventBus block#2843
piLurk wants to merge 1 commit intomainfrom
fix/connection-event-block

Conversation

@piLurk
Copy link
Copy Markdown

@piLurk piLurk commented Jun 27, 2023

Types

  • 🐛 Bug Fixes

Background or solution

eventBus 中长链接状态中断或错误消息一直被阻塞,无法发出:

#2828

ClientAppStateService 是一个标准的状态机,看起来设计之初并不保证下一个生命周期 的状态是链式的。因此这里应该使用 reachedAnyState 方法,而不是用reachedState方法。

Changelog

@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


wudongyue seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@opensumi opensumi Bot added the 🐞 bug Something isn't working label Jun 27, 2023
@erha19 erha19 requested a review from hacke2 June 27, 2023 14:15
@erha19
Copy link
Copy Markdown
Member

erha19 commented Jun 27, 2023

@piLurk 感谢贡献,麻烦签署一下评论内的 CLA 协议 ~

@codecov
Copy link
Copy Markdown

codecov Bot commented Jun 27, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.01 ⚠️

Comparison is base (8d9c284) 57.67% compared to head (d86ac87) 57.66%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2843      +/-   ##
==========================================
- Coverage   57.67%   57.66%   -0.01%     
==========================================
  Files        1336     1336              
  Lines       83923    83923              
  Branches    17455    17455              
==========================================
- Hits        48401    48397       -4     
- Misses      32286    32289       +3     
- Partials     3236     3237       +1     
Flag Coverage Δ
jsdom 52.69% <100.00%> (-0.01%) ⬇️
node 16.78% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/core-browser/src/bootstrap/connection.ts 48.14% <100.00%> (ø)

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@hacke2
Copy link
Copy Markdown
Member

hacke2 commented Jun 28, 2023

image

感觉最好还是修改下这里

@piLurk
Copy link
Copy Markdown
Author

piLurk commented Jun 28, 2023

@hacke2 感觉不应该改这里,这里的本意不是链式状态,调用的地方用 reachedAnyState 比较合适,有更多上下文?

image

感觉最好还是修改下这里

@piLurk
Copy link
Copy Markdown
Author

piLurk commented Jun 28, 2023

@piLurk 感谢贡献,麻烦签署一下评论内的 CLA 协议 ~

签署了

@Ricbet Ricbet mentioned this pull request Jun 28, 2023
1 task
@erha19
Copy link
Copy Markdown
Member

erha19 commented Jun 29, 2023

感觉不应该改这里,这里的本意不是链式状态,调用的地方用 reachedAnyState 比较合适,有更多上下文?

@piLurk 这里感觉 reachedStatereachedAnyState 方法二选一吧,虽然这块可能有一些设计,但以目前框架执行逻辑看,每个生命周期应该都是阶段性的,并且状态确定的(是否 resolved?),我建议还是移除 34 行逻辑,这里使用 reachedState 也是合理的。

@piLurk piLurk closed this Jul 18, 2023
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

None yet

Development

Successfully merging this pull request may close these issues.

4 participants