Skip to content

Type, document, and refactor Sync, port tests to Mocha #1331

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 47 commits into
base: master
Choose a base branch
from
Open

Type, document, and refactor Sync, port tests to Mocha #1331

wants to merge 47 commits into from

Conversation

raucao
Copy link
Member

@raucao raucao commented Jun 8, 2025 •
edited
Loading

Includes some small other improvements. However, this PR is mostly about properly typing and documenting sync, as well as making the functions more readable by using async/await in many places.

Noteworthy additional changes:

  • Adds a new event called "sync-started", when a periodic/background sync has started (but not when outgoing changes are synced immediately).
  • Emit conflict change events asynchronously, so app developers don't have to handle them in a special way, improve conflict event docs
  • Do not emit conflict events for nodes that were deleted both locally and remotely (d04f334)
  • Fix remote deletes not handled properly when folder has local changes (098962d)
  • For more, see referenced issues below

refs #1251 (where I added sub-issues that are all easier to tackle with these changes)
refs #1270
closes #1173
closes #839
fixes #1335
fixes #1204

P.S.: I rebased the branch as much as possible before opening the PR. But there are too many conflicts when trying to consolidate the rest even more.

raucao added 27 commits June 5, 2025 13:10
@raucao
WIP docs, async
e3f80ef
@raucao
Speed up specs, clear localStorage where used
2c97b61
@raucao
Use setItem for localStorage
60373af 
Missed this one in #1330
@raucao
Use null value instead of relying on parse exception
0a3cfd2 
`getItem` returns `null` (whereas `localStorage[SETTINGS_KEY]`) used to
return `undefined`.
@raucao
WIP Port Sync tests to Mocha
de9cce4
@raucao
Use correct type for RS class in Sync, fix type errors
e5c5f77
@raucao
WIP Types
3fad53a
@raucao
Remove unused argument and code path
c1494b6
@raucao
WIP docs, async
b22dc59
@raucao
Rename sync setup functions for clarity
70b352b
@raucao
Refactor, document doTasks; port tests for sync()
afaa82a
@raucao
Refactor, document task collect functions; port tests
8733e23
@raucao
Use Object.keys
2498392
@raucao
No explicitly public functions if not used outside of class
1c596c5
@raucao
Port more tests
f9e307f
@raucao
Types and docs for Sync
4724fcb
@raucao
Formatting
823e28f
@raucao
Add 'sync-started' event
f50278d 
closes #1173
@raucao
Finish porting sync tests
c6ea79c
@raucao
Refactor finishTask
a60d5bd
@raucao
Remove "legacy nodes" entirely
26cb53c
@raucao
More types and docs for Sync
004f607
@raucao
Use async/await
39e06d6
@raucao
Refactor handleGetResponse
ad636f9
@raucao
More types and docs for Sync
119f384
@raucao
Refactor autoMerge
0bf6ef6
@raucao
Improve function name
57b6054
@raucao raucao requested a review from a team June 8, 2025 14:09
@raucao raucao mentioned this pull request Jun 13, 2025
Closed
@raucao raucao mentioned this pull request Jun 13, 2025
Open
@raucao
Copy link
Member Author

raucao commented Jun 16, 2025 •
edited
Loading

Added a rather useful change for conflict events: handling them in apps doesn't require manually using setTimeout or similar anymore.

Also, My Favorite Drinks now has support for updating drinks and it will automatically handle conflicts:

https://github.com/remotestorage/myfavoritedrinks/blob/master/app.js#L50-L59

https://myfavoritedrinks.remotestorage.io

@raucao raucao force-pushed the feature/1251-sync branch from 012e75d to 91bf93d Compare June 16, 2025 09:36
@raucao
Emit conflict events asynchronously
16fbeb4 
This way, apps can use these the same way as they can use other change
events.
@raucao raucao force-pushed the feature/1251-sync branch from 91bf93d to 16fbeb4 Compare June 16, 2025 09:38
@raucao raucao mentioned this pull request Jun 16, 2025
Closed
raucao added 3 commits June 18, 2025 12:29
@raucao
Use async/await
fb6bba8
@raucao
Fix local itemsMap being inconsistent after changes on both ends
098962d 
This can cause documents to either not be deleted at all when they
should be, but at the least for the parent folder's `local` itemsMap to
become inconsistent and potentially persist forever, because it can
never be the same as the remote itemsMap again (which is the criterium
for removing it).
@raucao
Do not emit conflict events for mutual deletions
d04f334 
No changes can be lost when documents are deleted on both ends
@raucao raucao force-pushed the feature/1251-sync branch from 11ad749 to d04f334 Compare June 18, 2025 10:43
@raucao
Copy link
Member Author

raucao commented Jun 18, 2025

I was able to address almost all of the sub-issues of #1251 in this PR now. The couple of issues that are left open should be addressed in separate PRs after this one is merged.

@raucao
Always emit sync-done when done
6afc445 
It doesn't matter if a client received `sync-started` or not. When there
are no more tasks to run after any task has finished successfully, the
sync is done.

Also removes a superfluous second log statement for when sync is done.
@DougReeder
Copy link
Contributor

The tests pass on my dev machine, the tests appear to cover new/changed functionality, and there's no code smell. But I'm not familiar enough with the syncing code sign off on this.

@raucao
Copy link
Member Author

raucao commented Jul 16, 2025

@galfert Any chance you could have a look at this soon?

@raucao raucao mentioned this pull request Jul 25, 2025
Merged
@raucao
Copy link
Member Author

raucao commented Jul 27, 2025 •
edited
Loading

We'll have to merge this soon, because I cannot add any more PRs against this one. It's already too large.

@rosano
Copy link
Contributor

rosano commented Jul 28, 2025

We'll have to merge this soon, because I cannot add any more PRs against this one. It's already too large.

Since it's been about two months, I would ask or tag other core people individually and wait a day or two for any commitment to review, else merge.

@raucao
Copy link
Member Author

raucao commented Jul 29, 2025

/cc @michielbdejong @silverbucket

@silverbucket silverbucket self-requested a review July 29, 2025 08:18
@raucao raucao requested review from galfert and michielbdejong and removed request for a team July 29, 2025 08:30
silverbucket
silverbucket approved these changes Jul 29, 2025
View reviewed changes
Copy link
Member

@silverbucket silverbucket left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM, tests are all passing 👍

@rosano
Copy link
Contributor

rosano commented Jul 29, 2025

I tried CRUD on this rough build of hyperdraft and it seemed to sync fine between 2 private windows https://test-hyperdraft.remit.rosano.ca/en/write

@raucao
Copy link
Member Author

raucao commented Jul 29, 2025

Bonus: if your app uses change events with origin window, it should even work between browser tabs in the same browser session now (and instantly).

@rosano
Copy link
Contributor

rosano commented Jul 29, 2025

Bonus: if your app uses change events with origin window, it should even work between browser tabs in the same browser session now (and instantly).

Looking forward to implementing that in future apps, and maybe documenting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silverbucket silverbucket silverbucket approved these changes

@galfert galfert Awaiting requested review from galfert

@michielbdejong michielbdejong Awaiting requested review from michielbdejong

Assignees
No one assigned
Labels
dev environment improvement
Milestone
No milestone
5 participants
@raucao @DougReeder @rosano @silverbucket @darkdread