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

reportError function #11

Closed
rrsergeynikiforov opened this issue Oct 10, 2018 · 9 comments
Closed

reportError function #11

rrsergeynikiforov opened this issue Oct 10, 2018 · 9 comments
Labels

Comments

@rrsergeynikiforov
Copy link

rrsergeynikiforov commented Oct 10, 2018

I have a question

const queue1 = new Queue({ tasksRef, processTask:processTask1, reportError, options:options1 })

Why don't we use reportError?

async function processTask1(task) {
  try {
    // do the work and optionally return a new task
    doSomething();
  } catch (e) {
    reportError(e)
    throw e // this marks the task as failed
  }
}

like this, we are calling reportError manually.
in new Queue, Why don't we use reportError as parameter?

@EECOLOR
Copy link
Member

EECOLOR commented Oct 10, 2018

@rrsergeynikiforov Thank you for the input.

I am not sure I understand you correctly. Do you mean that instead of this:

async function processTask(task) {
   try { ... }
   catch (e) { reportError(e); ... }
}

You want to do this:

async function processTask(task, { reportError }) {
   try { ... }
   catch (e) { reportError(e); ... }
}

If that is the case I really like your suggestion! If you mean something else, please try to explain it some more, maybe with an example.

@rrsergeynikiforov
Copy link
Author

Thanks for your reply.
When creating a new queue, We're using like this.
const queue1 = new Queue({ tasksRef, processTask:processTask1, reportError, options:options1 })

Where is reportError function used?
Do we need to pass reportError as parameter?
or How can we call reportError by default without catch (e) { reportError(e); ... }?

@EECOLOR
Copy link
Member

EECOLOR commented Oct 11, 2018

Ahh, now I understand.

The reportError function is used (at the time of writing) in a few places:

https://github.com/kaliberjs/firebase-queue/blob/master/src/queue_worker.js#L21

newTaskRef.on('child_added', tryToProcessAndCatchError, reportError)

and

https://github.com/kaliberjs/firebase-queue/blob/master/src/queue_worker.js#L32

await claimAndProcess(ref).catch(reportError)

It is essentially a way for the library to report a problem. The original library swallowed a lot of errors. So if you, for example, had a problem in one of your rules, the original firebase-queue would pretend everything is ok while it was not working.

There is a slight difference between typing the try/catch and not typing it within the processTask function .

// set the error state on the task when an error occurs, but don't report it
async function processTask(task) {
   ...
}
// report the error and set the error state on the task when an error occurs
async function processTask(task) {
   try { ... }
   catch (e) { reportError(e); throw e }
}
// report the error and complete the task normally when an error occurs
async function processTask(task) {
   try { ... }
   catch (e) { reportError(e) }
}

In some use cases we do not put our code into a try/catch block because the error is part of the application flow. The client (that created the task) will give the user feedback, so there is no need to report the error (to rollbar).

We could theoretically add a queue option:

new Queue({ options: { reportErrorsFromProcessTask: true/false }, ... })

The following snippets would then be equivalent (the same):

new Queue({ options: { reportErrorsFromProcessTask: true }, ... })
...
async function processTask(task) {
   ...
}
new Queue({ options: { reportErrorsFromProcessTask: false }, ... })
...
async function processTask(task) {
   try { ... }
   catch (e) { reportError(e); throw e }
}

@itdev123
Copy link

itdev123 commented Oct 12, 2018

It's good
Thanks

@EECOLOR
Copy link
Member

EECOLOR commented Oct 13, 2018

@itdev123 I have moved you new question here: #12

@itdev123 are you and @rrsergeynikiforov the same person?

@EECOLOR
Copy link
Member

EECOLOR commented Oct 13, 2018

I have opened two feature requests that came from this thread, vote for them if you want to see them added: #13 #14

@geoervin
Copy link

@EECOLOR This explanation really cleared up error reporting for me.
#11 (comment)

It would be a great addition to the documentation.

@EECOLOR
Copy link
Member

EECOLOR commented Oct 13, 2018

@geoervin Good idea! The comment contained multiple sections. Would you be willing to adjust the documentation in way that makes it clear to you in a pull request?

@geoervin
Copy link

geoervin commented Oct 14, 2018 via email

@EECOLOR EECOLOR closed this as completed Oct 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants