TiddlyWiki5 icon indicating copy to clipboard operation
TiddlyWiki5 copied to clipboard

[5.4.0] Allow sync adapters to save/load/delete multiple titles at once

Open Arlen22 opened this issue 6 months ago • 13 comments

This PR adds save/load/delete tasks which allow a sync adapter to handle all pending saves/loads/deletes at once. This allows optimizations for certain scenarios where it is preferred to bring everything up to date at once rather than trickling tiddler operations through, potentially blocking subsequent syncing for much longer than intended.

Currently the syncer saves one tiddler at a time until all tiddlers are saved, then it syncs from the server, then it deletes all tiddlers that need to be deleted one at a time, then it loads all needed tiddlers one at a time. This check occurs after each operation, so if we are in the middle of loading 3 tiddlers, and suddenly need to save 10k tiddlers, the remaining tiddlers will not be loaded, and no further sync from server will occur, until all 10k tiddlers are saved. This is also rate-limited at 4 per second, although that can be changed, but in practice most web requests take at least that long if not longer.

The disadvantage of this is that most, if not all, storage locations could save 10k tiddlers in a few seconds at the longest, but saving one tiddler at a time tends to take much longer.

This PR allows sync adapters to opt into saving, loading, and deleting multiple tiddlers at once.

This adds the following to the existing sync adapter interface

interface SyncAdapter {
	/** Callback for the sync adapter to call to request a poll */
	subscribe?(cb: () => void): void;

	saveTiddlers?(options: {
		syncer: Syncer,
		tiddlers: Tiddler[],
		onNext: (title: string, adaptorInfo: any, revision: string) => void,
		onDone: () => void,
		onError: (err: Error) => void
	}): void;

	loadTiddlers?(options: {
		syncer: Syncer,
		titles: string[],
		onNext: (tiddlerFields: TiddlerFields) => void,
		onDone: () => void,
		onError: (err: Error) => void
	}): void;

	deleteTiddlers?(options: {
		syncer: Syncer,
		titles: string[],
		onNext: (title: string) => void,
		onDone: () => void,
		onError: (err: Error) => void
	}): void;
}

I basically just took the existing tasks and separated the callback code into onNext, onDone, and onError.

  • onNext is called for each title that is successfully processed to allow the syncer to update its own state. This matches existing behavior. For tiddlers where an error occurs, onNext should not be called. The existing syncer behavior does not care about tiddlers that failed because it will just try them again.
  • onError and onDone call the task callback.

I believe the behavior should be exactly identical to single tiddler tasks.

Arlen22 avatar Jun 26 '25 16:06 Arlen22

Deploy Preview for tiddlywiki-previews ready!

Name Link
Latest commit d6bd6b689708ffd56af40fdb9c4e0c44fcdea3b4
Latest deploy log https://app.netlify.com/projects/tiddlywiki-previews/deploys/69441abf9cd229000703dece
Deploy Preview https://deploy-preview-9117--tiddlywiki-previews.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

netlify[bot] avatar Jun 26 '25 16:06 netlify[bot]

Confirmed: Arlen22 has already signed the Contributor License Agreement (see contributing.md)

github-actions[bot] avatar Jun 26 '25 16:06 github-actions[bot]

📊 Build Size Comparison: empty.html

Branch Size
Base (master) 2529.0 KB
PR 2533.4 KB

Diff: ⬆️ Increase: +4.4 KB

github-actions[bot] avatar Jun 26 '25 16:06 github-actions[bot]

I did not read or test the code yet. -- But is this a preparation for the MWS plugin? For "batch" processing

pmario avatar Jun 26 '25 17:06 pmario

I did not read or test the code yet. -- But is this a preparation for the MWS plugin? For "batch" processing

I'm not sure. Server-sent events and other more advanced syncing mechanisms may introduce additional complexity that the syncer isn't built for. But yes, this allows batch processing and for now MWS would probably use it.

Arlen22 avatar Jun 26 '25 18:06 Arlen22

@Jermolene I'm marking this for 5.3.7 because the changes are quite simple and I'm targeting es5. It's up to you whether you want to include it this last minute.

Arlen22 avatar Jun 26 '25 18:06 Arlen22

I've added this to MWS for now so if anyone wants to test how it works you can open a codespace on this commit of MWS and then run the following commands

npm install
npm run prisma:generate
npm start init-store
npm start

Arlen22 avatar Jun 26 '25 18:06 Arlen22

I support addressing this need, but would prefer if we delayed until v5.4.0 to allow for further testing and potential refinement.

I have a GitHub sync adaptor that has been pending a rewrite to get around exactly these limitations and would benefit from batching updates to avoid the chances of rate limiting by the API. In my use case it would be most beneficial to be able to batch saves and deletes to the server in a single API call (a single git commit).

I am just recovering from knee surgery and will likely need a week or two before I am able to properly test these changes.

saqimtiaz avatar Jun 27 '25 08:06 saqimtiaz

I'm adding a subscribe function so the syncadapter can request the syncer to poll. This lets us keep the existing pattern of (mostly) not telling the syncadapter what syncer is using it.

Arlen22 avatar Jul 02 '25 17:07 Arlen22

Since 5.3.7 will be released on 7th of July, I don't think it will merge PR with new features. IMO this PR should target 5.4.0.

Leilei332 avatar Jul 03 '25 00:07 Leilei332

Since 5.3.7 will be released on 7th of July, I don't think it will merge PR with new features. IMO this PR should target 5.4.0.

Agreed, that's sooner than I was thinking, so yeah.

Arlen22 avatar Jul 03 '25 15:07 Arlen22

This also takes care of #3227.

Arlen22 avatar Nov 12 '25 19:11 Arlen22

@saqimtiaz ... IMO should be scheduled for v5.4.0

pmario avatar Dec 10 '25 19:12 pmario

@Jermolene @saqimtiaz where are we with this and can we merge it?

Arlen22 avatar Dec 17 '25 15:12 Arlen22

Hi @Arlen22 I notice that this PR weighs in at +4KB, which I think is problematic given that it doesn't it doesn't provide benefits with the integrated syncadaptor.

I am fairly sure that the task classes used by the syncer are encapsulated to the degree that we can radically change the implementation, and don't need to worry about backwards compatibility for this internal interface. Does that give and scope for a more radical refactoring that has better code reuse?

Jermolene avatar Dec 17 '25 17:12 Jermolene

📊 Build Size Comparison: empty.html

Branch Size
Base (master) 2529.0 KB
PR 2529.5 KB

Diff: ⬆️ Increase: +0.5 KB


⚠️ Change Note Status

This PR appears to contain code changes but doesn't include a change note.

Please add a change note by creating a .tid file in editions/tw5.com/tiddlers/releasenotes/<version>/

📚 Documentation: Release Notes and Changes

💡 Note: If this is a documentation-only change, you can ignore this message.

github-actions[bot] avatar Dec 17 '25 20:12 github-actions[bot]

@Jermolene good point. I've combined them now.

Arlen22 avatar Dec 17 '25 20:12 Arlen22

Thanks @Arlen22, that's a good improvements in the code size delta. However, it is still 2KB so I wondered if there was any scope for further refactoring so that syncadaptor plugins can themselves implement the syncer task classes. We'd add a new syncadaptor method that the syncer invokes during initialisation. The syncadaptor would return its own implementation of the syncer task classes, overwriting the default null implementation of the multi-tiddler classes.

Jermolene avatar Dec 18 '25 08:12 Jermolene

@Jermolene well if I'm going to refactor it that much I would just get rid of them and dump what's left straight into the function that calls them because it's already so paired down there's not much left anymore. I'll take a look at it today.

Arlen22 avatar Dec 18 '25 10:12 Arlen22

@Jermolene I simplified the code to just use functions instead of entire classes. I think that's about the simplest I'm going to be able to get it for now.

Arlen22 avatar Dec 18 '25 15:12 Arlen22