Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds onError handler to subscription #261

Merged
merged 1 commit into from
Jul 30, 2020
Merged

Conversation

zpalin
Copy link
Contributor

@zpalin zpalin commented Jul 29, 2020

We are having issues with handling errors thrown inside a subscription. I looked around and couldn't find any suggested way of handling this. I think this simple fix would get us what we need. If there is a better way of handling these types of errors please let me know.

Copy link
Collaborator

@chrisdrackett chrisdrackett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't use subscriptions so I'm not as up to date with what problem this is solving myself. I did have one question around if its valuable to keep the existing functionality an option.

@@ -112,17 +112,22 @@ export const MSTGQLStore = types
function subscribe<T = any>(
query: string | DocumentNode,
variables?: any,
onData?: (item: T) => void
onData?: (item: T) => void,
onError: (error: Error) => void = error => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make onError optional? Keep existing behavior if you don't pass it in, but if you do give access to the error?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick reply. So it is optional because I am supplying a default onError here and the default onError is just a simple function that throws the passed error. If you prefer I can make it optional and use a check inline and throw if the function isnt defined.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup, you are correct, I totally glanced over this line!

@chrisdrackett chrisdrackett merged commit 6d5a826 into mobxjs:main Jul 30, 2020
@zpalin zpalin mentioned this pull request Jul 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants