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

Allow specifying a topic when subscribing to a document #487

Merged
merged 26 commits into from
Apr 11, 2023

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Mar 28, 2023

What this PR does / why we need it?

multi

1. Subscribe to Document with Specified Target

In the case where there are multiple data (such as root.todos, root.counter, etc.)
in one document root, as in the example above, if you want to perform a specific task
only when root.todos is changed,

  • as-is) you have to traverse the changeInfo.paths in the event value
    and check if the path($.todos) has been changed.
  • to-be) You can specify the target path as $.todos
    and register a callback function to execute when the specified path is changed.
    So, in the example, the doc event occurs every time the document is changed,
    but the events for todos, counter, and quill are only triggered
    when their respective path or any of their nested values are changed.
// as-is
doc.subscribe((event) => {
  for (const changeInfo of event.value) {
    for (const path of changeInfo.paths) { 
      switch (path) {
        case path.startsWith('$.target'):
	   const key = path.split('.')[1];  // extract the key
           const target = doc.getRoot()[key];  
           // Do something...
           break;
      }
    }
  }
});

// to-be
doc.subscribe((event) => {
  // The callback will be called when the document is changed.(as before)
  // Do something...
});
doc.subscribe('$.target', (event) => { 
  // The callback will be called when the targetPath or any of its nested values change.
  const target = doc.getValueByPath('$.target') // you can get the value by path 
  // Do something...
});

2. Improvement of Event Value for Local/Remote Change

In document subscription, the event value provides an array of operations, which is difficult to use.
Provide useful information to the user from the operation.

  • as-is)
    • IncreaseOperation { parentCreatedAt: TimeTicket, executedAt: TimeTicket, value: Primitive }
  • to-be)
    • { type: 'increase', value: 1, path: '$.counter' }
doc.subscribe('$.target', (event) => { 
  // If the event.type is `local-change` or `remote-change`
  for (const changeInfo of event.value) {
    const {message, operations} = changeInfo;
    for (const op of operations) { 
      // ex) { type: 'increase', value: 1, path: '$.counter' }
      switch (op.type) {
        case 'increase':  
           // Do something...
           break;
      }
    }
  }
});

Any background context you want to provide?

What are the relevant tickets?

Fixes #445

Checklist

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

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #487 (2147252) into main (0afacc0) will increase coverage by 0.47%.
The diff coverage is 88.66%.

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
+ Coverage   89.71%   90.19%   +0.47%     
==========================================
  Files          69       69              
  Lines        5690     5985     +295     
  Branches      551      568      +17     
==========================================
+ Hits         5105     5398     +293     
+ Misses        390      381       -9     
- Partials      195      206      +11     
Impacted Files Coverage Δ
src/document/json/array.ts 87.75% <ø> (ø)
src/document/operation/operation.ts 100.00% <ø> (ø)
test/helper/helper.ts 82.22% <ø> (+8.14%) ⬆️
test/integration/object_test.ts 100.00% <ø> (ø)
src/document/operation/select_operation.ts 50.00% <42.85%> (+11.11%) ⬆️
src/document/operation/style_operation.ts 54.16% <50.00%> (+4.16%) ⬆️
src/document/operation/increase_operation.ts 66.66% <57.14%> (+2.38%) ⬆️
src/document/operation/add_operation.ts 72.22% <62.50%> (+1.63%) ⬆️
src/document/operation/move_operation.ts 66.66% <62.50%> (ø)
src/document/operation/set_operation.ts 66.66% <62.50%> (-3.93%) ⬇️
... and 10 more

... and 3 files with indirect coverage changes

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

`waitFor` continuously registers the listener, which may result in
missed event detections if an event occurs before the registration.
Therefore, `waitFor` has been replaced with `waitStubCallCount`
to verify the number of times the callback function is called
and which events are executed.
…cribe`

Modify the `selectPriv` method to return `TextChange` when setting the initial selectionMap
@chacha912 chacha912 marked this pull request as ready for review April 4, 2023 01:39
- `Modified` refers to the changes made when an operation is executed.
- `UpdateInfo` represents the changes made when `doc.update()` is called
  and it is the value exposed to the user.
- The `updates` field in `UpdateInfo` contains an array of `UpdateDelta`,
  which are the transformed values of `Modified`.
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.
Overall good. I left a few comments.

public/multi.html Outdated Show resolved Hide resolved
public/multi.html Outdated Show resolved Hide resolved
public/multi.html Outdated Show resolved Hide resolved
src/document/document.ts Outdated Show resolved Hide resolved
src/document/document.ts Outdated Show resolved Hide resolved
src/document/document.ts Outdated Show resolved Hide resolved
src/document/document.ts Outdated Show resolved Hide resolved
public/multi.html Outdated Show resolved Hide resolved
public/multi.html Outdated Show resolved Hide resolved
src/document/operation/operation.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.

LGTM.

@hackerwins hackerwins merged commit 059b20b into main Apr 11, 2023
1 check passed
@hackerwins hackerwins deleted the document-subscribe-topic branch April 11, 2023 11:02
hunkim98 pushed a commit to hunkim98/yorkie-js-sdk that referenced this pull request Jul 12, 2023
)

* Modify `doc.subscribe` function to allow specifying a topic

* Add document.getValueByPath

* Specify the doc.subscribe event value for each operation

* Use `waitStubCallCount` instead of `createEmitterAndSpy` and `waitFor`

`waitFor` continuously registers the listener, which may result in
missed event detections if an event occurs before the registration.
Therefore, `waitFor` has been replaced with `waitStubCallCount`
to verify the number of times the callback function is called
and which events are executed.

* Add test that `remote-change` event is properly detected in `doc.subscribe`

* Modify the `selectPriv` method to return `TextChange` when setting the initial selectionMap

---------

Co-authored-by: Youngteac Hong <susukang98@gmail.com>
@hackerwins hackerwins mentioned this pull request Sep 21, 2023
2 tasks
hackerwins added a commit that referenced this pull request Sep 21, 2023
Trie was initially introduced to reduce the number of events from a
common ancestor(#351). However, it was later removed during the
implementation of the topic subscribe feature(#487, #566).

For now, I'll remove unused Trie and reopen the event-reducing issue
again(#266).
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.

Provide a method for getting the value from the remote-change path
2 participants