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

Add error func to schedule #14

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

jorganisak
Copy link

@jorganisak jorganisak marked this pull request as draft July 3, 2020 20:24
@jorganisak jorganisak marked this pull request as ready for review July 3, 2020 20:46
@jorganisak jorganisak changed the title Add error emitter Add error func to schedule Jul 3, 2020
@mjmaurer
Copy link
Contributor

mjmaurer commented Jul 5, 2020

thanks for the PR! could you add a little more context to why this is needed? does the current errfunc not work?

@jorganisak
Copy link
Author

Sure!

The current errFunc works as expected without these changes for catching problems with polling. I added the passing of the errFunc to the schedule function to catch any problems that might happen in the function passed by the caller of pollWithInterval.

If an error happens in https://github.com/crownheightsaid/mutual-aid-app/blob/master/src/workers/airtable-sync/worker.js#L51, or any off the promises that are called after polling happens, it fails quietly and continues to poll.

Writing it out makes me think this PR could be a nice-to-have rather than something needed to deploy the code in crownheightsaid/mutual-aid-app#108

Or maybe we don't need it at all!

@mjmaurer
Copy link
Contributor

mjmaurer commented Jul 6, 2020

I'd like to see if its possible to fix crownheightsaid/mutual-aid-app#108 without these changes. I think the current API before and after this change would be the same, but it's possible I'm missing something!

@jorganisak
Copy link
Author

That sounds right to me 👍

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