Add invariant to let Flow understand BatchItemNotFound is a subclass of Error

[ryanruanwork]

In some case, item in resultGroups is an error rather than an array of object, see https://github.com/Yelp/dataloader-codegen/blob/v0.1.4/src/implementation.ts#L246-L251

We need to turn that Error object into an array of Error in order to match the number of item in requestGroups

[magicmark]

Hmm, does this case ever actually happen tho?

https://github.com/Yelp/dataloader-codegen/blob/master/src/implementation.ts#L287
https://github.com/Yelp/dataloader-codegen/blob/master/src/implementation.ts#L310

We check if a resource return value is an error or not, and replace it with a list of CaughtResourceErrors if so - which matches the current function type shape.

Have you observed this behaviour actually happening? If so I think that's a bug...but if typescript is complaining about it, maybe we're missing something. Are you able to trace how this could happen?

tl;dr I think the current shape that this function accepts is correct and should stay the same?

(worst case scenario we can write a guard at the top of the function and throw an error)

[ryanruanwork]

Hmm, does this case ever actually happen tho?

https://github.com/Yelp/dataloader-codegen/blob/master/src/implementation.ts#L287
https://github.com/Yelp/dataloader-codegen/blob/master/src/implementation.ts#L310

We check if a resource return value is an error or not, and replace it with a list of CaughtResourceErrors if so - which matches the current function type shape.

Have you observed this behaviour actually happening? If so I think that's a bug...but if typescript is complaining about it, maybe we're missing something. Are you able to trace how this could happen?

tl;dr I think the current shape that this function accepts is correct and should stay the same?

(worst case scenario we can write a guard at the top of the function and throw an error)

Well, you are right. I miss that part of code, I add an invariant check to make Flow understand that BatchItemNotFoundError is an Error

[magicmark]

Hmm why is this needed? We made response an error in the line just above

Here's a similar repro:

https://flow.org/try/#0PTAEDMBsHsHcCh4GNIEMDO7QDFrQKIBOh0hoApgB4Au5AdgCZZElkDeAvgNzLR3rUIeUAF5QdcrBx4WpABQByAJ4BLANbl0CgJQ94ICDCnpy5ALZYAFqgAONpaFgrql0C5XoAXPDng8n0FlCbURwAFc6JGoVPlBrRkhyILlyYgCg7VA2eABIAwZoUFQ3SxU6AHMAOmrQeA5EA1RGEo9veIZE5L9oXSA

Is just this a flow version thing? Or can you show a similar minimal repro that errors with our current logic?

[ryanruanwork]

Root cause comes from https://unpkg.com/dataloader-codegen@0.1.4/lib/runtimeHelpers.js.flow and search BatchItemNotFoundError
you can see declare export class BatchItemNotFoundError mixins Error, this is not correct, we should use extends rather than mixins

[ryanruanwork]

I think the way we generate Flow from TS using flowgen is not perfect https://github.com/joarwilk/flowgen/blob/80792a4f21c5e5707443a9d82550493b47c14885/src/printers/declarations.js#L299

[magicmark]

ah I see. Ok makes sense, thanks for doing the digging here!

Can we add a comment about why we're adding the invariant, linking to this conversation or something? Otherwise it looks redundant when reading the code.

(Bonus points if we can make a minimal repro and make a ticket on the flowgen project - not necessary tho)

After the comment I think we're good to ship!

[ryanruanwork]

Minimal repro is https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVATApgYxgQwCcsw98BncsAMTjgFFDC5CwBbASwA8OA7Kxs1YBvAL6oAFLQZMWALjCCWASiA, I will create an issue on the flowgen project.

yea, I will add some comments about the reason why I add redundant code here.

[magicmark]

Looks great, thanks!