ChiriVulpes / scryfall-sdk

A Node.js SDK for https://scryfall.com/docs/api, written in TypeScript.
MIT License
99 stars 16 forks source link

`.cancelAfterPage().waitForAll()` never resolves #52

Closed multimeric closed 2 years ago

multimeric commented 2 years ago

I believe I've identified a bug. If you use await Scry.Cards.search(some_query).cancelAfterPage().waitForAll();, the promise never resolves so the function hangs forever. This is because waitForAll only resolves on end: https://github.com/ChiriVulpes/scryfall-sdk/blob/b30b9defba7342229fee39a3458e2aec82dbe7d6/src/util/MagicEmitter.ts#L74-L83

However if the emitter gets cancelled due to cancelAfterPage(), we skip emitting end, even though the query has successfully completed: https://github.com/ChiriVulpes/scryfall-sdk/blob/b30b9defba7342229fee39a3458e2aec82dbe7d6/src/api/Cards.ts#L678-L679

Here is a reproducible example:

import * as Scry from "scryfall-sdk";

try {
    const cards = await Scry.Cards.search("lang:fr").cancelAfterPage().waitForAll();
    console.log(cards);
}
catch (e) {
    console.error(e);
}

It seems to immediately exit, because the promise never resolves or rejects, but also there are no pending events.

ChiriVulpes commented 2 years ago

Ah this is something my tests managed not to cover, somehow. I'll fix tonight

multimeric commented 2 years ago

Thanks, that would be great!

ChiriVulpes commented 2 years ago

Fixed as of scryfall-sdk@3.2.0

ChiriVulpes commented 2 years ago

You might want to use v3.2.1 instead that I just pushed, I think it's slightly closer to what I'd originally intended. Technically the tests passed for both but there may be an edge case that this should cover.

multimeric commented 2 years ago

Thanks! That definitely seems to fix it.