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

applyRemoteChanges() should be handled as promise. #11

Open
bud-mo opened this issue Apr 15, 2020 · 3 comments
Open

applyRemoteChanges() should be handled as promise. #11

bud-mo opened this issue Apr 15, 2020 · 3 comments

Comments

@bud-mo
Copy link

bud-mo commented Apr 15, 2020

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
Copy link
Author

bud-mo commented Apr 15, 2020

Also the clear parameters is not used.

@nponiros
Copy link
Owner

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

this.syncable.on('statusChanged', (status, url) => {
).

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.

@bud-mo
Copy link
Author

bud-mo commented Apr 15, 2020

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.

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

No branches or pull requests

2 participants