Closed marcopiii closed 4 years ago
@Yuudaari
🚧 not completed: the new MagicBiEmitter class is missing a *[Symbol.asyncIterator]
function, which causes the following error when there are multiple entries in not_found
(just try to run the tests)
(node:13851) UnhandledPromiseRejectionWarning: TypeError: not_found is not iterable
at Cards.<anonymous> (.../scryfall-sdk/out/api/Cards.js:274:42)
at Generator.next (<anonymous>)
at fulfilled (.../scryfall-sdk/out/api/Cards.js:5:58)
at processTicksAndRejections (internal/process/task_queues.js:97:5)
How that function works is not completely clear to me since there are two different sets of objects now, and unfortunately I have no more time to work on this at the moment.
Do you have suggestions about how to solve this?
As much as I hate to say it, this implementation kinda overcomplicated things a bit. It would have been better to discuss it first in an issue.
From the project README:
If you want to make large, complex changes, make an issue before creating a PR. If I disagree with the changes you want to make, and you've already made them all in a PR, it'll feel a lot worse than being shot down in an issue, before you've written it all.
Using a bit of what you seemed to need as the base, I've committed an implementation of vague "not found" support into the existing MagicEmitter, which is only used for the collection endpoint currently. It adds the not_found
stuff to the array in waitForAll
and it adds a separate notFound
generator. (I also cleaned a couple things up in that commit, btw 😛)
I intend to publish this as 1.6.4 since it's nonbreaking, but I'm checking with you first — does it satisfy what you needed it for?
As much as I hate to say it, this implementation kinda overcomplicated things a bit. It would have been better to discuss it first in an issue.
No problem at all. I started to tinker with the code just to understand if the feature was already available and I found myself writing this even before thinking to open an issue.
I intend to publish this as 1.6.4 since it's nonbreaking, but I'm checking with you first — does it satisfy what you needed it for?
Yes I did some test and it works good. The only thing that doesn't completely convince me is the Promise<T[] & { not_found: NOT_FOUND[] }>
type returned by waitForAll
because you can't have a valid JSON representation of it (if you try to JSON.stringify()
it the not_found
is omitted of course).
I understand that it's the only way to make the change not-breaking, but I think it's worth to be highlighted at least in the documentation.
Anyway, it does exactly what i need 👍
Yeah, will do. I'll mention it in the docs.
As for it not being JSONifiable, I had a few reasons:
not_found
property ie const jsonifiable = { data, not_found: data.not_found }
not_found
property (so doing this shouldn't make it harder to debug)Besides that I also had these two reasons:
waitForAll
simpler
Currently
Cards.collection (...collection: CardIdentifier[]): MagicEmitter<Card>
ignores thenot_found
field returned by Scryfall API, so there is no way to give a feedback to the user about the identifiers for which a result has not been found.I changed the
processCollection
to read that field too and use a newMagicBiEmitter<T, Q>
class which works likeMagicEmitter<T>
except that:not_found
events for Q values, to be used for each value of thenot_found
array of the API responsewaitForAll
method returns a{ data: T[], not_found: Q[]}
object (which is a breaking change)This new emitter is used only by collection method to leave other methods unchanged since its
waitForAll
returns a different type.I also created an
AbstractMagicEmitter
with the code shared byMagicEmitter
andMagicBiEmitter
to avoid code duplication (I'm open to suggestions about better names 😅).