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

Draft: Sync pause and resume #951

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

obedm503
Copy link
Contributor

Why Is This Useful

The SyncManager is an incredibly powerful addition to SignalDB but it is missing the ability to gracefully pause and resume sync.

I maintain a large application made up of many independent modules. Each module uses many collections: some shared, some unique. This application is long-lived. Users leave it running for hours or days at a time. Users have permission to access some or all the modules. All modules are not equally important to the user depending on their role. Users mostly use a few modules and rarely use the rest. Some modules are only used on an annual or quarterly basis.

This change is useful because it allows us to build applications where collections can become active or inactive throughout the lifetime of the session as the user navigates around the application. It would allow us to save both network and server resources by not syncing collections while they're not in use.

Summary of Changes

This is my implementation of sync pause and resume. This PR would introduce the following changes:

  • when a collection is in offline mode it will continue recording changes but will not attempt to "push" them
  • registerRemoteChange takes the collectionOptions just like push and pull
  • registerRemoteChange changes from executing once in the constructor to executing for each collection every time sync resumes
  • registerRemoteChange can return a clean-up function to allow for reconnection after a pause (think WebSockets)
  • renames syncAll to startSyncAll. startSyncAll fulfills the same purpose except it can be called multiple times
  • introduces startSync which calls registerRemoteChange and sync. This effectively replaces sync. sync is now private. Collections start in offline mode and only start syncing after startSync is called
  • introduces pauseSync to clean up remote-change subscriptions and put the collection into offline mode.
  • introduces pauseSyncAll to pause sync for all collections

For an example of how this change would affect an application please see this diff: obedm503/trellix-offline@90b0300

Practical Example

The initial state of the application:
initial-state

The application goes offline, web sockets are closed:
offline

The user makes changes while offline but sync is paused:
make-changes

The application goes back online, web sockets are reopened, and collections sync:
reconnect


This is very much a draft PR. I know you initially said you're not sure about changing how remote-change subscriptions happen, but I hope I made a good case for the change. I updated the tests so they all pass. I have not updated the documentation but will do so once this is a final PR. Also, I tried to maintain your code style, but please let me know if there are any style changes you wish to make. For the future, I recommend adopting something like Prettier to enforce your style in an automated way.

Related to #947

Copy link

codecov bot commented Sep 19, 2024

Codecov Report

Attention: Patch coverage is 63.82979% with 17 lines in your changes missing coverage. Please review.

Project coverage is 98.81%. Comparing base (fa2cedc) to head (2f4b397).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/signaldb/src/SyncManager/index.ts 63.82% 11 Missing and 6 partials ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##              main     #951      +/-   ##
===========================================
- Coverage   100.00%   98.81%   -1.19%     
===========================================
  Files           54       54              
  Lines         1401     1430      +29     
  Branches       321      331      +10     
===========================================
+ Hits          1401     1413      +12     
- Misses           0       11      +11     
- Partials         0        6       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@maxnowack
Copy link
Owner

Thanks for the PR @obedm503! 🙂
I'm quite busy right now, but I'll do a full review by the beginning of next week.

Some quick thoughts:
I'm unsure about the renaming of sync and syncAll. It seems like the new startSync and startSyncAll functions are calling registerRemoteChange every time they are executed and cleaning up the old one if it's present. I can think of a scenario where you only want to trigger a manual sync without tearing down and reinstantiating the whole connection.

My suggestion would be to keep sync and syncAll as they are, and just toggle a flag indicating whether a collection should be synced in startSync/pauseSync, in addition to the logic for registerRemoteChange. This would allow the user to pause/unpause the sync while keeping manual syncing possible. It would also avoid the breaking change of renaming the functions. What are your thoughts on this?

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