dumbmatter / fakeIndexedDB

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

Transactions/Cursors fail with `jsdom` #67

Closed ph-fritsche closed 2 years ago

ph-fritsche commented 2 years ago

With jsdom environment in jest@27 acting on a transaction after allowing other code to run (per await) throws errors. This differs from executing the same calls in a node environment.

Maybe this is related to #64

https://github.com/ph-fritsche/repro-jest-jsdom-indexeddb

dumbmatter commented 2 years ago

Thanks for the repo with a reproduction!

It does seem like it could be related to #64... but if I put jest.useRealTimers(); at the top of your test script, it still fails. So maybe it's something else jsdom is doing, not the timers? I dunno. I've never really used it before.

Anyone have any ideas? If it was as simple as using real timers, I could do it like suggested in #64 probably, but it doesn't seem to be that simple...

ph-fritsche commented 2 years ago

The problem seems to be here: https://github.com/dumbmatter/fakeIndexedDB/blob/be0551ea9049e25dedc13ccfe6bba9c33f64228d/src/FDBDatabase.ts#L220

This starts and completes the transaction per setImmediate. But setImmediate executes the callback before process.nextTick in jsdom.

const tx = db.transaction('foo', 'readwrite')
await new Promise(r => process.nextTick(r))
// with node environment: { _state: 'active' }
// with jsdom environment { _state: 'finished' }

I've confirmed the following workaround in the repro:

global.setImmediate = cb => setTimeout(cb, 0)

Would it be feasible to use setTimeout in the the Database.processTransactions() implementation?

dumbmatter commented 2 years ago

In general it is difficult to switch between the timing functions as described in #64. It may be possible in this specific case, I'm not sure, but before I even get there... it doesn't seem to help here. You are correct that global.setImmediate = cb => setTimeout(cb, 0) at the top of your test file makes it work. But changing the setImmediate call to setTimeout in repro-jest-jsdom-indexeddb/node_modules/fake-indexeddb/build/lib/Database.js does not get rid of the error. Not sure why, I guess the timer replacement is doing something sophisticated that I don't understand.

Like... any idea about why jest.useRealTimers(); doesn't seem to help? Is jest/jsdom still messing with the timers even after that?

ph-fritsche commented 2 years ago

There are no fake timers in place. jest.useRealTimers() has no effect. (And I think if you use fake timers, you would just have to advance them.)

Replacing setImmediate with setTimeout in build/lib/Database fixes test "transaction is active on next tick". Also replacing setImmediate in build/FDBTransaction then fixes the initial two tests.

The clean solution might be to configure the timer to be used per setup.

import { setNextMacroTask } from 'fake-indexeddb/auto'
setNextMacroTask(setTimeout)
ath0mas commented 2 years ago

After update from v3.1.3 to v3.1.4 that contains only this fix, my Mocha tests started failing 😢 Error: Timeout of 2000ms exceeded. ... I'll try to create a reproduction repo before opening corresponding issue

ph-fritsche commented 2 years ago

@ath0mas Do you have fake timers in place that might need to be advanced so that the calls to setTimeout in https://github.com/dumbmatter/fakeIndexedDB/commit/17c15473e0dd61a32240431573d21a1b4f5b150e are resolved?

joshkel commented 2 years ago

@ath0mas I'm running into some test timeout issues too; I opened #69 with my findings so far.