Create powersync persister
Summary
Following on the heels of https://github.com/tinyplex/tinybase/pull/131, this is the PR to add the PowerSync persister to TinyBase. I'm submitting this in draft mode to give you a preview @jamesgpearce. Unfortunately I'm struggling to get the Jest unit tests working because the PowerSync SDK relies on Workers which are not available in Jest.
How did you test this change?
Implemented the persister in a React Native demo app: https://github.com/bndkt/powerchat-tinybase/blob/tinybase/src/app/(app)/tinybase.tsx
Jest unit tests are failing, like described above.
Thanks for the feedback again @jamesgpearce! Regarding the test, I tried using jsdom-worker, tried mocks, but couldn't get it to run. I spoke to PowerSync, they're working on making it work with Jest themselves, so maybe in the future there will be a chance to add this. What I did for now is a bit unconventional, curious what you think. In the end PowerSync is just using a sqlite database locally and the sync part with their service wouldn't be part of the tests anyways. So I created a mock (https://github.com/bndkt/tinybase/blob/46dcf0e96fed009963294cfc2c366d606f206413/test/unit/persisters/sqlite.ts#L52-L118, happy to move this into a separate file) which uses a simple sqlite database (just like the sqlite persister) but mimics the PowerSync interface. Now I "only" get 14 failing tests which are related to granular saves and the difference in SQL statements. I'll have to spent a few brain cycles on granular saves and the missing ON CONFLICT capability in PowerSync, will post about that separately.
I'm hanging out here but take it out of draft when you want me to take a final look
Hey @bndkt is this ready? 😄 - do you want me to review and/or rebase this for you?
Hey @jamesgpearce, thanks for checking in! I've poured a lot of time into fixing the tests now, but I'm afraid I need your help at this point. If you run the unit tests in this PR, you'll find 15 failing test. From my understanding, those can be put into two buckets:
- If you set useOnConflict to true for the PowerSync persister (exactly here), there's only 3 tests that fail, all of which are related to autoLoad. Those must be due to a problem with my PowerSync mock implementation but I can't figure out what the problem is.
- The remaining 12 failures are test for partial persists. I tried to implement partial changes without using ON CONFLICT (basically following the "good but tedious" option from here but end up with a lot of tedious code and am not really sure about the value of this. Do you think it would be feasible to just don't write partial updates for this persister? From what I understand this would only mean rewriting some columns that are unchanged, or am I missing something?
OK I'll take a look! I found some broken tests in the v5 beta branch and this might be similar. Leave it with me.
@bndkt please take a look at https://github.com/tinyplex/tinybase/pull/133/commits/51f1a2989e0dbafa87135cd40aec6f28566c44d4 - I simplified and isolated a simple (JSON-based) test that fails. The second write to the table (that has "c1":2) does not fire a change and so the test fails.
I read here that there should not be more than one instance per database file. Maybe that's why you had the factory pattern, but it still doesn't seem to be a singleton. However, that didn't seem to fix it.
How well does this mock work do you think?
@bndkt a gentle ping to see if you have any thoughts on this mock issue
@jamesgpearce Thanks for isolating this! I'm looking into it now, and see how to improve the mock in order to get the tests to pass.
@jamesgpearce in order to debug the mock function, I've temporarily enabled "on conflict" usage for the persister, so that it should behave exactly like the SQLite3 persister. I now get 2 failing tests. I've added comments to those and will tag you. They both seem to be timing-related, do you have any idea why the timing could be off here?
OK, I fixed it. There was something strange about adding and removing event listener references on the database. The solution was removeAllListeners in the mock. Should all pass 100%.
Think we're ready to merge now, @bndkt ?
Oh wow, thanks for discovering this @jamesgpearce! I'll have a final look now and then set it to ready for review.
Good to go @jamesgpearce!
Alright! Let's do any final polish in main.
Thanks so much for your work on this!