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

Clean up methods related to presence #599

Merged
merged 9 commits into from
Aug 4, 2023
Merged

Clean up methods related to presence #599

merged 9 commits into from
Aug 4, 2023

Conversation

chacha912
Copy link
Contributor

What this PR does / why we need it?

This is a follow-up task of #574

Any background context you want to provide?

image

When detaching, there are two possible scenarios:

When UserA detaches, a change is made to remove the presence, and the watch stream is canceled.

  1. If UserB receives the unwatched event first, UserB will remove UserA from onlineClients and trigger the unwatched event.

  2. If UserB receives the pushpull response first, which includes a change to remove presence, UserB will trigger the unwatched event beforehand while detaching since it involves unwatching.

What are the relevant tickets?

Fixes #

Checklist

  • Added relevant tests or not required
  • Didn't break anything

@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #599 (b2eb6d4) into main (1decb55) will decrease coverage by 0.38%.
The diff coverage is 96.42%.

@@            Coverage Diff             @@
##             main     #599      +/-   ##
==========================================
- Coverage   87.52%   87.15%   -0.38%     
==========================================
  Files          81       81              
  Lines        7953     8048      +95     
  Branches      816      818       +2     
==========================================
+ Hits         6961     7014      +53     
- Misses        679      720      +41     
- Partials      313      314       +1     
Files Changed Coverage Δ
src/client/client.ts 75.17% <66.66%> (-1.92%) ⬇️
src/document/document.ts 76.27% <90.00%> (+0.78%) ⬆️
test/integration/presence_test.ts 88.93% <97.93%> (-11.07%) ⬇️
test/helper/helper.ts 67.92% <100.00%> (-13.71%) ⬇️
test/integration/client_test.ts 99.65% <100.00%> (ø)
test/integration/document_test.ts 98.98% <100.00%> (ø)

@chacha912 chacha912 changed the title Add presence to unwatched event Clean up methods related to presence Aug 2, 2023
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. 👍

test/integration/presence_test.ts Outdated Show resolved Hide resolved
@hackerwins hackerwins self-requested a review August 4, 2023 02:15
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

Dealing with asynchronous events used to be challenging, but it seems that presence and document.subscribe events are now stable.

However, there are still some non-deterministic behaviors in our test cases. We need to address them to be deterministic in the next tasks.

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