Unable to use in tests that mock timers (e.g. overriding globalThis.setImmediate)

[bryan-codaio]

fakeIndexedDB/src/lib/scheduling.ts

Lines 20 to 29 in 4c01ec3

	// Schedules a task to run later. Use Node.js's setImmediate if available and
	// setTimeout otherwise. Note that options like process.nextTick or
	// queueMicrotask will likely not work: IndexedDB semantics require that
	// transactions are marked as not active when the event loop runs. The next
	// tick queue and microtask queue run within the current event loop macrotask,
	// so they'd process database operations too quickly.
	export const queueTask: (fn: () => void) => void =
	globalThis.setImmediate ||
	getSetImmediateFromJsdom() ||
	((fn: () => void) => setTimeout(fn, 0));

The code persists a reference to globalThis.setImmediate (which is always defined in our test setup). We have other logic (custom logic, but conceptually similar to jest's fake timers: https://jestjs.io/docs/timer-mocks) that will replace globalThis.setImmediate to give tests more control over timing. However, fake-indexeddb will continue to use the original setImmediate and thus break our testing logic.

I can solve this in our setup by patching the code with something akin to

export const queueTask = (fn: () => void): void => {
    const setImmediate = globalThis.setImmediate || 
        getSetImmediateFromJsdom() || 
        ((fn: () => void) => setTimeout(fn, 0));
    setImmediate(fn);
}

so it's respecting the current value of setImmediate rather than keeping a cached reference.

I'll go ahead and create a PR with that concept as a starting point for discussion, but let me know if it's flawed in some way (e.g. perhaps it's not performant enough).

[dumbmatter]

Thanks for the PR!

It doesn't seem to cause any performance issues. At least, the test suite doesn't slow down on your PR branch.

But once people start messing with the Node.js internals, it's hard to say what implications a change like this could have. So this would probably be a semver major change. Unfortunate timing that I just did a semver major release 2 weeks ago, it would have been nice to roll this into that! I'm not sure I want to do another so fast, unless there are more people affected by this issue.

So for now, could you just use your branch, or use patch-package to apply your patch? And then I will hopefully remember to put this in the next semver major release.

[bryan-codaio]

yeah for now we've patched this locally so as long as it eventually™ gets merged/released, we'll be happy! Ty for the quick response!

[julienw]

Just so that I understand fully: does it make fakeIndexedDB use a mocked setImmediate, or does it make always use the non-mocked setImmediate?

If this is the former, I wonder if this is the right tool: I understand you'd like to control better when fakeIndexedDB returns or something like that, but doesn't this export too much the internals (that fakeIndexedDB uses setImmediate for example)?

I'm not sure, just asking to get more opinions too :-)

[bryan-codaio]

Just so that I understand fully: does it make fakeIndexedDB use a mocked setImmediate, or does it make always use the non-mocked setImmediate?

I'm updating the code to always fetch the current value for setImmediate, whether that's mocked or not. So if some other code (like jest's fake timers) updates that, then fakeIndexedDB will use the updated version. I.e. our test initialization ordering won't be quite so fragile, if fakeIndexedDB happens to initialize before we mock the timers.

So... I think the answer to your question is "neither".

If this is the former, I wonder if this is the right tool: I understand you'd like to control better when fakeIndexedDB returns or something like that, but doesn't this export too much the internals (that fakeIndexedDB uses setImmediate for example)?

I'm not necessarily trying to get more control over fakeIndexedDB, just trying to make it behave like the rest of the code being tested. I.e. if we've mocked timers so that we can better control when time progresses, it was problematic that fakeIndexedDB would just continue chugging along because it was using the original setImmediate, and thus it got out of sync with other code and broke some assumptions.

[bryan-codaio]

@dumbmatter now that some months have passed, would it make sense to merge #93 and create another major release for it?

[dumbmatter]

It's in v6.0.0. I am still a little scared about this one!