-
Notifications
You must be signed in to change notification settings - Fork 95
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
Add RealtimeSyncOff and refactor interface of SyncMode #772
Conversation
…nto sync-off-mode
There was a problem hiding this 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.
From my understanding, the current structure of the SDK's SyncMode is as follows:
- Realtime: With EventStream
- PushPull
- PushOnly
- SyncOff
- Manual: Without EventStream
- PushPull
- PushOnly
- SyncOff
Even in Manual mode, users can set PushOnly or SyncOff, which may confuse them. How about refactoring SyncMode to have a flat structure for easier understanding?
Proposed Sync Mode options:
- SyncMode.Manual
- SyncMode.Realtime(same with `PushPull`)
- SyncMode.RealtimePushOnly
- SyncMode.RealtimeSyncOff
I think we need to keep Promise
in the return and operate asynchronously only when changing from Manual to Realtime.
Perhaps the APIs will also change as shown below:
client.changeSyncMode(doc, SyncMode.Manual); // prev: await client.pause();
client.changeSyncMode(doc, SyncMode.Realtime); // prev: await client.resume();
client.changeSyncMode(doc, SyncMode.RealtimePushOnly); // prev: client.pauseRemoteChanges(doc);
client.changeSyncMode(doc, SyncMode.RealtimeSyncOff); // prev: client.resumeRemoteChanges(doc);
@hackerwins Thank you for the suggestion. I agree that refactoring |
@hackerwins I've implemented your suggestions. Additionally, would it be better to change the |
@chacha912 That's a good idea. I think it will be more convenient to use. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. 👍
Thank you for handling my requests during the review.
Please update the document too.
https://yorkie.dev/docs/js-sdk
SyncOff
mode
What this PR does / why we need it?
The
SyncOff
mode will allow users to maintain the watch stream functionality while disabling synchronization.Does this PR introduce a user-facing change?:
Any background context you want to provide?
SyncOff
mode, the existing functions (pauseRemoteChanges
,resumeRemoteChanges
) have been unified underchangeRealtimeSyncMode
.changeRealtimeSyncMode
is currently designated for use only in realtime sync. Therefore, if you want to resume from manual mode topushonly
mode, you need to callresume()
and then change the sync mode. By default,resume()
operates in push-pull mode, but would it be necessary to provide an option to set the sync mode during the resume process?What are the relevant tickets?
Fixes #770
Checklist