dumbmatter / fakeIndexedDB

A pure JS in-memory implementation of the IndexedDB API
Apache License 2.0
562 stars 69 forks source link

Ref of Observable from liveQuery is not reactive #81

Closed simonmarshall closed 1 year ago

simonmarshall commented 1 year ago

hi, thanks for providing fake-indexeddb.

i noticed that when testing a store using fake-indexeddb, a ref created from an observable of a liveQuery was not reactive.
when not using fake-indexeddb, ie, when not in a test context, the ref is reactive as expected.

to reproduce this, i construct a store with setup including:

  const database = new TestDatabase();
  const getAllRows = () => database.testTable.toArray();
  const dataRef: Readonly<Ref<TestData[]>> = useObservable(from(liveQuery(getAllRows)), { initialValue: [] });

ie, getAllRows is a function returning a promise for all rows in the table and dataRef is a Ref constructed from that function as a liveQuery

with an empty database table, if my test looks like this it will fail:

    await store.updateData([testData]);
    expect(store.dataRef).toEqual([testData]); // this will fail but dataRef should have updated

with an empty database table, if my test looks like this it will succeed:

    await store.updateData([testData]);
    expect(await store.getAllRows()).toEqual([testData]); // calling getAllRows() causes dataRef to update
    expect(store.dataRef).toEqual([testData]); // this will succeed

i've attached code to reproduce this:

what am i doing wrong?! ref-of-observable-from-livequery-is-not-reactive.zip

dumbmatter commented 1 year ago

If you're willing to help debug a bit more...

Are you able to make a smaller reproduction that just uses dexie, and doesn't include rxjs/vue/pinia? I'm just trying to narrow it down because I've never used any of those packages before, and it seems like this rxjs/vue/pinia would probably not be directly related to the actual bug, right?

simonmarshall commented 1 year ago

hi @dumbmatter - i can remove pinia easily enough, you're right it's not related to the bug, but less so @vueuse/rxjs, since i use its useObservable() to create the Ref from the liveQuery Observable. is that enough for you to go on?

attached is the test case without using pinia.

ref-of-observable-from-livequery-is-not-reactive.zip

simonmarshall commented 1 year ago

hi @dumbmatter - i've removed the dependency on @vueuse/rxjs, by copying a cut-down version of useObservable(), though it still uses vue's ref(). i can still reproduce the issue, so i presume nothing i cut-down is a factor.

i've extended testing with a test that updates a clean database table 3 times, with all combinations of calling/not calling getAllRows() after an update before testing dataRef. so there are 8 tests altogether. only the test which calls getAllRows() after every update will pass.

when i run i get:

npm i --ignore-scripts
"C:\Program Files\nodejs\npm.cmd" exec -- vitest run -t "Data Store" src/fidb.store.spec.ts

 RUN  v0.29.8 C:/Users/xxx

 ❯ src/fidb.store.spec.ts (8)
   ❯ Data Store (8)
     × updateData successfully updates dataRef when calling getAllRows() first: false false false
     × updateData successfully updates dataRef when calling getAllRows() first: false false true
     × updateData successfully updates dataRef when calling getAllRows() first: false true false
     × updateData successfully updates dataRef when calling getAllRows() first: false true true
     × updateData successfully updates dataRef when calling getAllRows() first: true false false
     × updateData successfully updates dataRef when calling getAllRows() first: true false true
     × updateData successfully updates dataRef when calling getAllRows() first: true true false
     ✓ updateData successfully updates dataRef when calling getAllRows() first: true true true

as described in the original report, dataRef should be updated regardless of calling getAllRows() first, such that all tests should pass.

attached is the complete project, let me know if you need anything else.

ref-of-observable-from-livequery-is-not-reactive.zip

dumbmatter commented 1 year ago

I tried playing around with it for like 10 minutes, but it's tricky to work with multiple libraries I'm not familiar with, I just wind up getting errors I don't understand. Ideally what I would do first to debug is make a minimal example of fake-indexeddb and dexie liveQuery together, to see if they work in isolation. Because it still seems strange that you would only see this bug when vue and vitest are added to the mix, idk what they might have to do with it.

If fake-indexddb and dexie liveQuery work fine in isolation, then I'd guess the problem is not with fake-indexeddb. If they don't work fine in isolation, it'd be easier for me to debug something with just those two libraries.

simonmarshall commented 1 year ago

hi @dumbmatter - attached is a cut-down version that only depends on dexie & fake-indexeddb (and uses jest not vitest).

it fails in the same way as before, ie, there are 8 tests and only the test which calls getAllRows() after every update will pass.

ofc, i/we can't be sure that the real impls of ref() & useObservable() aren't necessary to properly expose the underlying issue, but hopefully this will make it much easier for you to debug.

ref-of-observable-from-livequery-is-not-reactive.zip

dumbmatter commented 1 year ago

I think there is some race condition going on here, I suspect it's not the fault of fake-indexeddb although I'm not sure.

This code:

import { setTimeout } from "timers/promises";
import "fake-indexeddb/auto";
import Dexie, { liveQuery } from "dexie";

const addAwait = true;
console.log(`addAwait=${addAwait}`);

class FundsOfFundsDatabase extends Dexie {
  constructor() {
    super("fubar");
    this.version(1).stores({
      testTable: "foo",
    });
  }
}

const database = new FundsOfFundsDatabase();
const getAllRows = () => {
  return database.testTable.toArray();
};
const clearData = () => {
  console.log("clearData");
  return database.testTable.clear();
};
const updateData = (rows) => {
  console.log("updateData", rows);
  return database.testTable.bulkPut(rows);
};

console.log("\n# Set up liveQuery and subscription");
const observable = liveQuery(() => {
  console.log("liveQuery callback");
  return getAllRows();
});
observable.subscribe({
  next: (val) => {
    console.log("subscription returned value", val);
  },
});

if (addAwait) { await setTimeout(100); }

const testData1 = {
  foo: "foo",
};
const testData2 = {
  foo: "bar",
};
const testData3 = {
  foo: "fubar",
};

console.log("\n# Run tests");

if (addAwait) { await setTimeout(100); }

await clearData();

if (addAwait) { await setTimeout(100); }

await updateData([testData1]);

if (addAwait) { await setTimeout(100); }

await getAllRows();

if (addAwait) { await setTimeout(100); }

await updateData([testData2]);

if (addAwait) { await setTimeout(100); }

await getAllRows();

if (addAwait) { await setTimeout(100); }

await updateData([testData3]);

if (addAwait) { await setTimeout(100); }

await getAllRows();

Has this output:

addAwait=true

# Set up liveQuery and subscription
liveQuery callback
subscription returned value []

# Run tests
clearData
liveQuery callback
subscription returned value []
updateData [ { foo: 'foo' } ]
liveQuery callback
subscription returned value [ { foo: 'foo' } ]
updateData [ { foo: 'bar' } ]
liveQuery callback
subscription returned value [ { foo: 'bar' }, { foo: 'foo' } ]
updateData [ { foo: 'fubar' } ]
liveQuery callback
subscription returned value [ { foo: 'bar' }, { foo: 'foo' }, { foo: 'fubar' } ]

That all looks correct - you run updateData, you trigger the liveQuery subscription callback function, and you get the latest data.

But change const addAwait = true; near the top of that file to const addAwait = false; and you get this output:

addAwait=false

# Set up liveQuery and subscription
liveQuery callback

# Run tests
clearData
subscription returned value []
liveQuery callback
updateData [ { foo: 'foo' } ]
subscription returned value []
liveQuery callback
subscription returned value [ { foo: 'foo' } ]
updateData [ { foo: 'bar' } ]
liveQuery callback
subscription returned value [ { foo: 'bar' }, { foo: 'foo' } ]
updateData [ { foo: 'fubar' } ]
liveQuery callback
subscription returned value [ { foo: 'bar' }, { foo: 'foo' }, { foo: 'fubar' } ]

Now the subscription callback function is running slower than you're executing new database queries. Which isn't necessarily a problem, except your test code was assuming that wouldn't happen, so your tests were failing.

So why would this happen with fake-indexeddb and not with the browser? Could be a bug in fake-indexeddb, or could just be that the browser implementation runs a little slower in this case. I'm not sure. But given all the automated tests fake-indexeddb has about query/transaction ordering, I think it's more likely that it's the latter. I'm not totally sure though.

I don't think it's a bug in Dexie, I don't think they can guarantee how fast the subscription responds, your application code can synchronously make multiple database queries at the same time. But again, I'm not totally sure, maybe awaiting a query is supposed to guarantee the subscription callback fires before the next query? That seems impossible too though, since a transaction could still be open and block the subscription callback from finishing.

simonmarshall commented 1 year ago

hi @dumbmatter - thanks for looking into this and sorry my delayed reply. i think you are correct. i guess my await on getAllRows() was always long enough for the observable's livequery to have fired. i can make my test pass if i await until the expected post-update condition is met via a setInterval function, rather than an await on getAllRows().

i'll close this ticket on the basis of "operator error", thanks again.