Sound storage with SQLite #5157
Conversation
gnprice
added
a-redux
a-data-sync
|
OK, and the Jest tests are working! I wrote a small mock version of expo-sqlite for use in tests -- or really a fourth platform-specific implementation of expo-sqlite, for Node, to go along with the ones it already has upstream for Android, iOS, and web. In each of those cases there's something exposing SQLite to JS, and then a small JS file to adapt that binding's interface to the one expo-sqlite wants to present: for web it gets that binding from the browser, and for the other two expo-sqlite provides its own bindings, which make up most of the code in the package. So, here, I used https://www.npmjs.com/package/sqlite3 for the binding and then there's a small JS file to adapt that library's interface to the one expo-sqlite presents. That means that in our tests that exercise AsyncStorage, for example the round-trip test in Now that that's something we can do, I'll want to:
|
We'll be adding a bit more code in this area shortly. "Storage" makes a more apt name for this than "boot".
TODO perhaps drop the jest.config.js change, since it seems to not actually work under Jest anyway
Studied the implementation and confirmed that this array doesn't get mutated.
TODO tests. This should do everything we need, when installed fresh. Just needs some code to migrate data from the old AsyncStorage.
TODO this breaks storage-test.js... presumably because expo-sqlite has no Jest mock (how could it -- the mock would have to simulate SQLite) and apparently doesn't run in the Jest environment. The easy fix is to mock out our own AsyncStorage for the sake of this test; that really isn't any regression in test coverage from the status quo with the upstream AsyncStorage mocked out. The ideal fix would be to get expo-sqlite running in Jest (surely it isn't hard to get SQLite from Node, right?), so that we can have tests exercise our AsyncStorage code. That would be especially good for the sake of its migration code, which will be under-exercised in manual testing once this series itself is merged.
TODO libdef
Fixing this will have very little practical effect; the original, buggy, migration was released nearly a year ago, so almost all users that were going to run it will have already run it by now. But now that we've written tests for these migrations and we know it's buggy, it's pretty easy to fix. And doing so lets us clean up the test of the broken behavior, and also record our assessment of the consequences of the bug. (Which aren't especially bad, fortunately.)
If you're going to be skipping over several versions, you probably need more generality than mkSimpleMigration anyway.
|
OK! I've now gotten a good picture of what app-level migrations can look like with this system. Detailed discussion in chat: I've been pushing to this same branch as I go, but a lot of those changes won't belong in the first PR. So the next step is that I'll take the changes that make up what we'd want to merge as a first phase -- the SQLite bindings, the AsyncStorage replacement on top of them, and the migration from legacy AsyncStorage, or perhaps a subset even of all that -- and clean those up into a PR. This PR thread has gotten quite long with the list of all my commits in this branch: 106 of them at this point. (Including many that will get squashed before this is mergeable.) So I'm going to close this draft PR before doing all that branch-revision. I'll open a new PR when that first phase is more or less in mergeable shape. |
gnprice
force-pushed
the
dev-sqlite
branch
2 times, most recently
from
b8f2a76
to
720a30f
Compare
|
(I forgot to close this before updating the branch with some changes. So I just reopened it, pushed to the branch the same commit it'd been at as of my last comment, and then closed it again. That should mean that the PR stays pointing to that WIP series of 106 commits as I go and revise the branch.) |
Here's a prototype to fix #4841, storing soundly in SQLite.
This uses expo-sqlite as the library to make SQLite conveniently accessible from our JS code in React Native, and then provides a small bit of code that provides the same interface as the old AsyncStorage library but with a SQLite database always as the backing store, following more or less the plan from #4841 (comment) . It includes a migration to bring the user's data over from the legacy AsyncStorage.
I did a quick manual test on Android, and it worked great:
Still to do before this can be merged:
Manually test on iOS too.
This version breaks our Jest suite, because expo-sqlite doesn't properly run there.
We've been mocking out the legacy AsyncStorage library, so the simplest solution would be to mock out our replacement AsyncStorage. I might do that just to get this unblocked and merged.
An improved version would be to get expo-sqlite running in Jest, so that we can have tests exercise our AsyncStorage code. That would be especially good for the sake of its migration code, as migrations tend to be under-exercised by manual testing. It's no surprise that there isn't a handy mock for expo-sqlite -- a mock would basically have to simulate SQLite itself. But surely there are SQLite bindings available for Node (which is the JS engine Jest uses)… and given those, it shouldn't be too hard to get expo-sqlite running in Node, and then under Jest.
I'd like to prototype first what our future app-level migrations will look like on top of this. That doesn't necessarily need to be complete before merging this, but it may make sense to coalesce the system for those with what this has to manage the migration from the legacy AsyncStorage, and in that case it may influence how we'd like this migrations table to look.
There's a commit toward the end that downgrades expo-sqlite, to stay before the "unimodules" ->
expomigration. I'll want to either drop that, if we've done that migration before this is otherwise ready to merge, or squash it with the commit adding expo-sqlite in the first place, if we haven't.(And see comments below.)