kaliberjs / firebase-queue

A trimmed and more robust version of the original Firebase Queue
MIT License
20 stars 3 forks source link

reportError function #11

Closed rrsergeynikiforov closed 5 years ago

rrsergeynikiforov commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

It's good Thanks

EECOLOR commented 5 years ago

@itdev123 I have moved you new question here: https://github.com/kaliberjs/firebase-queue/issues/12

@itdev123 are you and @rrsergeynikiforov the same person?

EECOLOR commented 5 years ago

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

geoervin commented 5 years ago

@EECOLOR This explanation really cleared up error reporting for me. https://github.com/kaliberjs/firebase-queue/issues/11#issuecomment-428839371

It would be a great addition to the documentation.

EECOLOR commented 5 years ago

@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 commented 5 years ago

I will give it a try. 😀

On Sat, Oct 13, 2018 at 1:51 PM EECOLOR notifications@github.com wrote:

@geoervin https://github.com/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?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kaliberjs/firebase-queue/issues/11#issuecomment-429561709, or mute the thread https://github.com/notifications/unsubscribe-auth/ALW7KFaYVGMFoKnQF88t9G0Iy3xDbOPRks5ukigTgaJpZM4XWFHR .