-
Notifications
You must be signed in to change notification settings - Fork 11
Refactor update hooks #105
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good, and cleaner than the old approach.
One thing to confirm: Does throttledUpdatedTables
fire the events asynchronously, or is that also synchronous since it's using updatesSync
? Or since it's actually only firing on the commit stream, it would be async?
I'm asking because I've seen issues in other SDKs where we fired updates immediately in the commit hook, which actually runs before the commit has completed. That then caused edge cases where watch queries would still see the old state before the commit. I don't think that would be an issue here, but just need to confirm.
Then for this:
So we'd have to coordinate this release with a PowerSync SDK update.
- To what extent does it affect non-PowerSync users? Would that just generally require re-building/re-downloading the worker and WASM files when updating this package?
- What exactly does "coordinate this release with a PowerSync SDK update" mean? Does it just mean we also need to update the workers and wasm there when the powersync SDK updates the sqlite_async dependency?
And if the sqlite_async dependency could be updated automatically and break things (without for example updating the powersync version), should we do a major version bump for this?
Yes, that's the only thing required.
Yes, and I agree that doing a major bump here to avoid PowerSync users upgrading this package without a compatible PowerSync upgrade sounds reasonable. |
Since it only fires in response to a timer or an event on |
On second thought, given this issue below, I think we can release this as just a minor update (or patch, since this is still 0.x?): It's not going to help anyone if we make this a new major release. |
Well, my idea was to release a patch release of our Dart SDK with the updated version shortly afterwards. Given that users have to update their workers, I think I prefer compilation errors over exceptions due to mismatching workers that may be hard to understand. |
This refactors how update hooks are dispatched to clients:
autocommit
status to determine when a transaction ends - we're now doing that with commit hooks.updatesSync
stream that invokes listeners synchronously. This allows us to fold update hooks withO(affected_tables)
memory, instead of having the Dart SDK allocateO(affected_rows)
async tasks to dispatch hooks.On the web, I've also refactored how we dispatch update hooks: Instead of debouncing the
updates
stream internally, we're now using custom requests to open and close streams. The worker will send a custom request to clients for each collected update notification.It's worth noting that this will require a coordinated update of workers:
sqlite3.wasm
built with version 2.7.0 ofpackage:sqlite3
(because necessary helper functions are missing). So that's the minimum version we support now, but we've shipped that bundle for a while in PowerSync.package:sqlite_async
. So we'd have to coordinate this release with a PowerSync SDK update.Closes #106