tinybase icon indicating copy to clipboard operation
tinybase copied to clipboard

Create powersync persister

Open bndkt opened this issue 1 year ago • 12 comments

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.

bndkt avatar Feb 21 '24 11:02 bndkt

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.

bndkt avatar Feb 22 '24 02:02 bndkt

I'm hanging out here but take it out of draft when you want me to take a final look

jamesgpearce avatar Feb 24 '24 18:02 jamesgpearce

Hey @bndkt is this ready? 😄 - do you want me to review and/or rebase this for you?

jamesgpearce avatar Mar 02 '24 13:03 jamesgpearce

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?

bndkt avatar Mar 05 '24 07:03 bndkt

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.

jamesgpearce avatar Mar 05 '24 15:03 jamesgpearce

@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?

jamesgpearce avatar Mar 11 '24 20:03 jamesgpearce

@bndkt a gentle ping to see if you have any thoughts on this mock issue

jamesgpearce avatar Mar 22 '24 14:03 jamesgpearce

@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.

bndkt avatar Mar 25 '24 05:03 bndkt

@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?

bndkt avatar Mar 26 '24 03:03 bndkt

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 ?

jamesgpearce avatar Mar 28 '24 20:03 jamesgpearce

Oh wow, thanks for discovering this @jamesgpearce! I'll have a final look now and then set it to ready for review.

bndkt avatar Apr 02 '24 05:04 bndkt

Good to go @jamesgpearce!

bndkt avatar Apr 02 '24 11:04 bndkt

Alright! Let's do any final polish in main.

jamesgpearce avatar Apr 04 '24 15:04 jamesgpearce

Thanks so much for your work on this!

jamesgpearce avatar Apr 04 '24 16:04 jamesgpearce