Fix issue where subscriptions had no error handling

[clayne11]

Explicitly handle the error case similar to how it is done for
mutations.

Checklist:

• Make sure all of the significant new logic is covered by tests

[hwillson]

Thanks for submitting this PR @clayne11! This change makes sense, but I'm a bit worried about backwards compatibility. We might have to wait until we do a major version bump, to get this merged. I'll think about it a bit more, but regardless thanks again!

[clayne11]

I don't agree with this logic. Regardless of whether or not the observer has an error handler we shouldn't be sending the result to the next handler.

If there is an error handler we should call it, otherwise we should return. The error case should never trickle down to the next handler.

[hwillson]

Yes, that's a mistake. I'm not crazy about seeing return in a forEach as its intent can be misleading, so I was in the process of removing it but clearly didn't finish. I'll adjust - thanks!

[hwillson]

Wait a sec @clayne11 - maybe we can handle this in a backwards compatible way. How about this: If an error handler exists, we call it with the error, and don't call next. If it doesn't, we keep the existing approach of letting the error case trickle down to next. This way people that are already expecting the error to trickle down to next won't be affected (that much). They either have an error handler defined which will now be used, or they don't have one defined and are checking for the error in the result, which will still be there. I think this should cover all of our bases, and let us get this merged now. I'll re-hash the code - let me know what you think.

[clayne11]

I'm open to this solution. If it helps us get this merged more quickly let's do it!

[clayne11]

@hwillson it's not really a breaking change IMO, it's a bug fix. There was improper handling of errors before. We're not able to update from graphql@0.11.7 until this gets merged.

It would be great to see this get out sooner than later.

[hwillson]

I agree that it's a bug fix, but I'm worried that people have gotten used to working around this bug, and are expecting to deal with the error in the incoming result themselves. These changes will switch that around on them, which might have an unexpected outcome for people. That's why I'm considering it a backwards incompatible change (even though it's a bug fix). I'm happy to be persuaded if you think otherwise though - I'd love to see this get merged.

UPDATE: See #3800 (comment); I think we should be able to get this merged.

[clayne11]

@hwillson do you think we can get this merged and released today?

[hwillson]

Done - thanks @clayne11!

[clayne11]

@hwillson are you planning to cut a release with this soon?

[hwillson]

@clayne11 Fingers crossed, really soon. I'll try to get a release out today.

[hwillson]

@clayne11 I haven't forgotten about this; I just want to wrap up #3817 before pushing the next AC version. I'm almost done.

[clayne11]

Is it onerous to cut a release? Would it be possible to cut one now then another when you finish that other PR?

[hwillson]

These changes should be live now @clayne11 (turns out we're going to have to cut an alpha release for #3187). Thanks!

[clayne11]

Thank you!

…

On Fri, Aug 17, 2018, 10:06 Hugh Willson ***@***.***> wrote: These changes should be live now @clayne11 <https://github.com/clayne11> (turns out we're going to have to cut an alpha release for #3187 <#3187>). Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3800 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACs0VDesJ5IY-TX5WZ4CTvBQ29ETCS__ks5uRs3zgaJpZM4V4Rmf> .