sync_client icon indicating copy to clipboard operation
sync_client copied to clipboard

`applyRemoteChanges()` should be handled as promise.

Open bud-mo opened this issue 4 years ago • 3 comments

According to the source code of applyRemoteChanges

His invocation at poll_sync_protocol.js#L40 should be handled as promise to be sure to follow the correct workflow.

bud-mo avatar Apr 15 '20 10:04 bud-mo

Also the clear parameters is not used.

bud-mo avatar Apr 15 '20 10:04 bud-mo

Hey and thanks for the issue.

From what I remember and from looking at the code I'm pretty sure that the current implementation is the way I wanted it at the time. You are right that the code doesn't wait on the promise but that shouldn't make any difference. I guess we could call onSuccess after the promise fulfilled to avoid calling again the server before the client is done with the current changes but I don't remember this causing any issues. In case applyRemoteChanges is rejected internally then the whole process stops anyway and if I remember correctly Dexie.Syncable informs the client of this (See https://github.com/nponiros/sync_client/blob/103f1ca04b6ce3201f58086443a9a6b27fb161ca/src/sync_client.js#L42).

Does the current behaviour of the library cause any issues?

Yes you are correct that clear is not used and I guess this might cause issues for users using TypeScript or similar. In JavaScript it doesn't cause any issue but is not really 'clean' to use it like this. I will try to fix it as soon as possible.

nponiros avatar Apr 15 '20 14:04 nponiros

Thank You for the reply.

I don't know if this will cause any issue, I am not using this library.

I used the code of the poll_sync_protocol.js file as reference to write a custom sync protocol for Dexie, while digging into the code I found this little issue.

bud-mo avatar Apr 15 '20 14:04 bud-mo