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

Add RealtimeSyncOff and refactor interface of SyncMode #772

Merged
merged 12 commits into from
Apr 11, 2024
Merged

Conversation

chacha912
Copy link
Contributor

@chacha912 chacha912 commented Apr 9, 2024

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?:

- 1️⃣ With the addition of the `RealtimeSyncOff` mode, the `SyncMode` has been simplified into four options. Usage is as follows:

// as-is
await client.pause(doc); // manual sync
await client.resume(doc); // realtime sync(pushpull)
client.pauseRemoteChanges(doc); // pushonly 
client.resumeRemoteChanges(doc); // pushpull 

// to-be
await client.changeSyncMode(doc, SyncMode.Manual);
await client.changeSyncMode(doc, SyncMode.Realtime); // Same as PushPull mode
await client.changeSyncMode(doc, SyncMode.RealtimePushOnly); 
await client.changeSyncMode(doc, SyncMode.RealtimeSyncOff); // Added RealtimeSyncOff mode 

- 2️⃣ In manual sync mode, the `sync()` function can now only be executed in pushpull mode.

await client.sync(doc, SyncMode.PushOnly) // Deprecated: Only pushpull synchronization is supported now.

- 3️⃣ The option to specify manual sync during attachment has been updated.

// as-is 
await client.attach(doc, { isRealtimeSync: false });

// to-be
await client.attach(doc, { syncMode: SyncMode.Manual });

Any background context you want to provide?

  • I'd like to discuss the user interface
  1. With the addition of SyncOff mode, the existing functions (pauseRemoteChanges, resumeRemoteChanges) have been unified under changeRealtimeSyncMode.
// as-is
client.pauseRemoteChanges(doc); // pushonly 
client.resumeRemoteChanges(doc); // pushpull 

// to-be
client.changeRealtimeSyncMode(doc, SyncMode.PushPull); // pushpull
client.changeRealtimeSyncMode(doc, SyncMode.PushOnly); // pushonly
client.changeRealtimeSyncMode(doc, SyncMode.SyncOff); // syncoff
  1. The changeRealtimeSyncMode is currently designated for use only in realtime sync. Therefore, if you want to resume from manual mode to pushonly mode, you need to call resume() 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?
// manual -> realtime(pushonly)
// as-is
await client.pause(doc); // manual sync
await client.resume(doc); // realtime sync
client.changeRealtimeSyncMode(doc, SyncMode.PushOnly);

// provide option
await client.pause(doc); // manual sync
await client.resume(doc, {mode: SyncMode.PushOnly}); // realtime sync

image

What are the relevant tickets?

Fixes #770

Checklist

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

@chacha912 chacha912 marked this pull request as ready for review April 9, 2024 06:11
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.

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);

@chacha912
Copy link
Contributor Author

chacha912 commented Apr 11, 2024

@hackerwins Thank you for the suggestion. I agree that refactoring SyncMode into a flat structure is a good idea. So, if I understand correctly, in manual mode, only push-pull synchronization is possible? Previously, users could perform push-only synchronization even in manual mode using the sync(doc, syncMode) function. In that case, I'll also remove the syncMode option from the sync function.

@chacha912
Copy link
Contributor Author

@hackerwins I've implemented your suggestions. Additionally, would it be better to change the isRealtimeSync option to syncMode in the attach function as well?

@hackerwins
Copy link
Member

@chacha912 That's a good idea. I think it will be more convenient to use.

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. 👍

Thank you for handling my requests during the review.
Please update the document too.
https://yorkie.dev/docs/js-sdk

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide syncoff mode to maintain watch stream without syncing in realtime sync mode
2 participants