Skip to content
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

Fix optimistic updates #1727

Merged
merged 8 commits into from
May 30, 2023
Merged

Fix optimistic updates #1727

merged 8 commits into from
May 30, 2023

Conversation

dfahlander
Copy link
Collaborator

The unit tests for dexie-react-hooks failed. This PR resolves the failures that happened and makes the test work simpler by also functioning in cache mode (when the liveQueries updates immediately).

The issues found in dexie's liveQuery and cache-middleware was the following:

  • useObservable() will launch the querier immediately on component render to test if it could give out a synchronous value. This is generic code in order to support synchronous observables but isnt' very good with liveQuery observable since useLiveQuery() uses useObservable(liveQuery()) internally and useObservable() forces it to run a dummy query on every initial render of a component. This is optimized to not do that by inspecting hasValue() of the observable and if that function is present and returning false, don't care to try getting a synchronous value.
  • The above behavior in combination with that we now abort the transaction when unsubscribing, caused the cache to store a failing promise.
  • When the querier is executed next time (to actually care about what's coming), it will find the cached promise and wait for it (it doesn't know quite yet that it will be failing with AbortError). When it eventually fails, the valid liveQuery fails and the component throws and the ErrorBoundary is shown.
  • Tried to workaround this by re-executing the query in case it was from the cache and fails. Still got error, this time Transaction is inactive. Turned out the reason for this is that the original transaction was lost while waiting for the promise in the cache (that eventually failed). So we actually had to launch a new and fresh readonly transaction when this happens. This solved the entire issue.
  • Then, I also optimized so that useObservable doesn't try to subscribe to liveQuery observables immediately, but instead waits until the effect is launched and only fires queries towards indexedb when they are actually being observed.

I have also a corresponding PR for master-3 (dexie@3.x) that enables the hasValue() method on returned observables so that the next version of dexie-react-hooks will optimize also for dexie@3.

useObservable will try subscribe and unsubscribe immediately on observables to get a possible initial value. This is really contraproductive for liveQueries that will never have an initial value but instead fire off a dummy request for every useLiveQuery().
Next version of dexie-react-hooks will check for observable.hasValue() before doing this behavior and skip it in case the observable doesn't have an initial value (is asynchronous). Thereby saving performance for live queries.
If the observable has hasValue() method, check it. If returns
Better than doing the dance every time, or following copilot's buggy suggestions to not even check the indexOf position before using it in splice()
error when testing invalid key in the 4th test.
@dfahlander dfahlander merged commit f22cd1e into master May 30, 2023
1 check passed
@dfahlander dfahlander deleted the fix-optimistic-updates branch May 30, 2023 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant