raidmulator: incomplete Promise check
Souma-Sumire opened this issue · 6 comments
Description
When the type is 'StartsUsing', everything works fine, but when the type is something else, an error occurs. And if I add 'delaySeconds', there is no error. I don't quite understand.
I attempted to fix it in HERE, but I'm unsure why 'StartsUsing' is an exception.
Additional information
Realization
Options.Triggers.push({
zoneId: ZoneId.MatchAll,
id: "souma_test",
triggers: [
// promise verification passed
{
id: "Test StartsUsing noDelay",
type: "StartsUsing",
netRegex: {},
promise: async (data, matches) => {},
},
// Console: Trigger Test Ability: promise function did not return a promise
{
id: "Test Ability",
type: "Ability",
netRegex: {},
promise: async (data, matches) => {},
},
// Console: Trigger Test GameLog: promise function did not return a promise
{
id: "Test GameLog",
type: "GameLog",
netRegex: {},
promise: async (data, matches) => {},
},
// Console: Trigger Test GainsEffect: promise function did not return a promise
{
id: "Test GainsEffect",
type: "GainsEffect",
netRegex: {},
promise: async (data, matches) => {},
},
// promise verification passed
{
id: "Test Ability Delay",
type: "Ability",
netRegex: {},
delaySeconds: 1, // differences
promise: async (data, matches) => {},
},
{
id: "Test GameLog Delay",
type: "GameLog",
netRegex: {},
delaySeconds: 1, // differences
promise: async (data, matches) => {},
},
{
id: "Test GainsEffect Delay",
type: "GainsEffect",
netRegex: {},
delaySeconds: 1, // differences
promise: async (data, matches) => {},
},
],
});
This is happening because the current zone when importing encounters from the indexedDB is within the dexie scope, so dexie's Promise
is overriding the built-in Promise
.
I believe the correct resolution for this would be to escape that zone after fetching the encounter from Persistor
, in the two cases where it's relevant in raidemulator.ts
(lines 222 and 233). I'm not sure exactly how to go about this other than wrapping the if (enc)
block in a setTimeout
or something, though, and that seems like a pretty ugly solution.
Oh, I see. Dexie overwrites the Promise
global object (as well as the Promise
constructor even on the previous object???), and so Promise.resolve
is really DexiePromise.resolve
and it wraps a native promise in a DexiePromise
. Normally Promise.resolve
returns the original object if it's instanceof Promise
. DexiePromise.resolve
does the same but only for DexiePromise
objects. Triggers with delays get an extra constructed delay promise that wraps everything else and so skip this issue. I ran a normal log through this and see plenty of StartsUsing
trigger errors as well, so there isn't anything special there.
If I breakpoint the Promise.resolve(promise)
line in popup-text.js
, it seems to be fine while going through different character perspectives (I see a lot of Loading id souma_test
in the console) and then at some point the Promise
global object gets overwritten. It's unclear to me when this happens as dexie.mjs
seems to run that code immediately. It seems like really bad form to overwrite the global Promise
object but it looks like even current versions of Dexie still use this due to IndexedDB promise incompatibility.
One nice thing about checking for Promise
exactly is that we know that both it thennable but also that it is asynchronous. It would be nice to have stronger guarantees about which functions created asynchronicity vs were always synchronous when looking at a cactbot trigger. Unfortunately, there's no way to guarantee that a function is asynchronous without running it and checking that it returns a promise, but promises themselves are guaranteed to be asynchronous.
I guess I see a couple of options:
- explicitly check for
instanceof DexiePromise
as well (like the commit mentioned in the description) - just check that the value of a trigger
promise
field has athen
function and live with (unlikely but possibly) synchronous triggerpromise
fields?
(cc @valarnin)
If you check the call stack, in all cases where it errors the top-level of the call stack will be in dexie's get
function. Any function running under this scope will have Promise
overridden with the DexiePromise
object.
From the dexie Promise
documentation:
window.Promise is always safe to use within transactions, as Dexie will patch the global Promise within the transaction zone, but leave it untouched outside the zone.
Basically, this code is calling the promise resolver while still within the transaction zone:
cactbot/ui/raidboss/emulator/data/Persistor.ts
Lines 61 to 67 in 31ef788
public async loadEncounter(id: number): Promise<Encounter | undefined> {
return new Promise<Encounter | undefined>((res) => {
let encounter: Encounter | undefined = undefined;
void this.transaction('readwrite', [this.encounters, this.encounterSummaries], async () => {
encounter = await this.encounters.get(id);
}).then(() => res(encounter));
});
}
What about this? Based on https://dexie.org/docs/Dexie/Dexie.transaction(), it seems like the callback to transaction is in the zone, but if you defer until after the transaction is done then it will no longer be in the zone.
Maybe this commented version will help with understanding?
public async loadEncounter(id: number): Promise<Encounter | undefined> {
return new Promise<Encounter | undefined>((res) => {
// Start a new transaction with the `encounters` and `encounterSummaries` tables
void this.transaction('readwrite', [this.encounters, this.encounterSummaries],
// Running from within that transaction, so now `window.Promise` has been overwritten with `DexiePromise`
async () => {
// Call `resolve` handler with the result of the `get` for the encounter
// Any code directly resulting from this call is executed within the transaction's scope/zone
res(await this.encounters.get(id));
}
);
});
}
I was thinking something even more simplified as such:
public async loadEncounter(id: number): Promise<Encounter | undefined> {
let enc: Encounter | undefined;
await this.transaction('readwrite', [this.encounters, this.encounterSummaries], async () => {
enc = await this.encounters.get(id);
});
return enc;
}
But I have no time right now to actually test any of these changes.
I think we're on the same page. That seems like it works. I'll put up a PR.