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

Clarify the ClientEvent that is sent to client.subscribe #464

Merged
merged 13 commits into from
Mar 6, 2023

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Feb 23, 2023

What this PR does / why we need it?

Clarify the ClientEvent that is sent to client.subscribe

  • Remove runWatchLoop when activating the client
  • Add sending of connected and disconnected events

PeersChanged Event

  • Elaborate on the value of the PeersChanged event

  • as-is

// js-sdk
type PeersChangedValue<P> = Record<string, Record<string, P>>;
  • to-be
// js-sdk
type PeersChangedValue<P> = {
  type: 'initialization' | 'watched' | 'unwatched' | 'presence-changed';
  peers: Record<DocumentKey, Array<{ clientID: ActorID; presence: P }>>;
};

// example
client.subscribe((event) => {
  switch (event.type) {
    case 'status-changed': {
      // event.value: activated, deactivated
      break;
    }
    case 'stream-connection-status-changed': {
      // event.value: connected, disconnected
      break;
    }
    case 'document-synced': {
      // event.value: synced, sync-failed
      break;
    }
    case 'document-changed': {
      // event.value: docKey[]
      break;
    }
    case 'peers-changed': {
      const { type, peers } = event.value;
      // type: initialization, watched, unwatched, presence-changed

      switch (event.value.type) {
        case 'initialization': {
          displayCursor(peers[doc.getKey()]); 
          break;
        }
        case 'unwatched': {
          // we can get which peer has unwatched.
          removeCursor(peers[doc.getKey()]);
          break;
        }
        case 'watched':
        case 'presence-changed': {
          // we can partially update for changed peers.
          updateCursor(peers[doc.getKey()]);
          break;
        }
      }

      // if you need entire peers
      displayPeers(client.getPeersByDocKey(doc.getKey()));
      break;
    }
  }
});

ClientEvent flow in test code

  • Add test for ClientEvent flow in client.subscribe

image

Any background context you want to provide?

What are the relevant tickets?

Fixes #

Checklist

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

@chacha912 chacha912 marked this pull request as ready for review February 23, 2023 09:26
@chacha912 chacha912 requested a review from hackerwins February 23, 2023 09:33
@hackerwins hackerwins marked this pull request as draft February 27, 2023 01:45
@chacha912
Copy link
Contributor Author

The test fails in CI and I've analyzed the cause. To resolve this issue, I think the order of events must be guaranteed. Please let me know if you have any good ideas.

The reason for the test failure is as follows:
Pasted Graphic 64

In the event flow diagram above, you can see that when clientA attaches the second document(doc2) in events 6-7, the stream connection is disconnected and reconnected.
At this point, clientB, who is subscribed to doc1, receives unwatched and watched events from clientA, but the order of these events is not guaranteed.
Therefore, if the events come in the order of watched, unwatched (in the case where the order of events 5-6 in clientB is reversed), clientB's peerPresenceMap information will only have {doc1: {peerPresenceMap: {clientB: {presence}}} left.
image

Then, when clientA detaches doc1, clientB receives an event that A has become unwatched.
To send the event to the user that clientA has become unwatched, the peer information is looked up in the peerPresenceMap. However, an error occurs because clientA does not exist in the peerPresenceMap.

@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Merging #464 (1838159) into main (7ddcf92) will increase coverage by 0.22%.
The diff coverage is 95.88%.

@@            Coverage Diff             @@
##             main     #464      +/-   ##
==========================================
+ Coverage   89.28%   89.50%   +0.22%     
==========================================
  Files          67       67              
  Lines        5254     5375     +121     
  Branches      524      528       +4     
==========================================
+ Hits         4691     4811     +120     
  Misses        381      381              
- Partials      182      183       +1     
Impacted Files Coverage Δ
src/document/document.ts 79.00% <ø> (ø)
test/helper/helper.ts 70.83% <85.18%> (+14.31%) ⬆️
src/core/client.ts 77.52% <94.23%> (+1.66%) ⬆️
test/integration/client_test.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@chacha912 chacha912 marked this pull request as ready for review March 3, 2023 10:00
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. The structure and types of PeersChanged have been cleaned up more in this PR. 👍

I left a few comments.

src/core/client.ts Outdated Show resolved Hide resolved
src/core/client.ts Outdated Show resolved Hide resolved
src/core/client.ts Show resolved Hide resolved
test/integration/client_test.ts Outdated Show resolved Hide resolved
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 reply. Left additional questions.

src/core/client.ts Outdated Show resolved Hide resolved
src/core/client.ts Outdated Show resolved Hide resolved
Users can get all peers using client.getPeersByDocKey(doc.getKey()),
so it is sufficient to only have peers(changedPeers info).
@hackerwins hackerwins self-requested a review March 6, 2023 07:40
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. 👍

@hackerwins hackerwins self-requested a review March 6, 2023 07:41
@hackerwins hackerwins merged commit cea0709 into main Mar 6, 2023
@hackerwins hackerwins deleted the client-event branch March 6, 2023 07:42
hackerwins pushed a commit that referenced this pull request Mar 6, 2023
- Remove runWatchLoop when activating the client
- Add sending of connected and disconnected events PeersChanged Event
@chacha912 chacha912 mentioned this pull request Mar 22, 2023
2 tasks
hunkim98 pushed a commit to hunkim98/yorkie-js-sdk that referenced this pull request Jul 12, 2023
…am#464)

- Remove runWatchLoop when activating the client
- Add sending of connected and disconnected events PeersChanged Event
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.

2 participants