add component and params to Error on visibility timeout

[pklingem]

According to MDN the error object is intended to have properties set on it, that the properties match what airbrake uses is a nice side-effect.

[dbertram]

Nit-pick: leftover extra spaces

[dbertram]

Combine this with the previous line?

[dbertram]

This lgtm, but can/should we set this up for all errors in processJob instead of only for timeout errors?

The new code plus the final .catch(finish) in processJob could be refactored to use a common addErrorContext (or whatever you want to call it) that sets error.params and error.component. Then any error thrown during process job would include the job payload, type, etc.

What do you think?

[pklingem]

good points, all. Addressing.

[pklingem]

@dbertram updated, had to introduce another synonym for done 🤦‍♂️ .

[dbertram]

I don't think there's a params in scope at this point...which is probably my fault for suggesting you combine those two lines earlier where iirc, you had a separate const params = ....

[flintinatux]

Yeah, I'm not seeing any params anywhere. Do we have linting for this library?

[dbertram]

I'm not saying it's any clearer, but is it worth making this a pipe?

const logTimeout = partial(timeoutLogger, [ params ])
const handleTimeout = pipe(
  always(new Error(timeoutErr.error)),
  addErrorContext,
  tap(logTimeout),
  complete
)

[flintinatux]

👀

[flintinatux]

@pklingem @dbertram: I've made some minor comments below, but I'd also like to address my main concern with this approach. The processJob function originally just selected a handler function for a job type, lifted it into a Promise chain, and then ran it. Now about 90% of that function is for handling timeouts, and is exceptionally imperative, despite the abundance of ramda involved. So I think we need a different approach.

I'd be more amenable to an approach that let us take advantage of the Promise chain, like this:

const assign = flip(Object.assign)

const reject = bind(Promise.reject, Promise)

const wrapError = params =>
  pipe(
    unless(is(Error), Error),
    assign({
      component: 'squiss-jobs'
      params
    }),
    reject
  )

const handleJob = ({ type, payload }) =>
  handlerFor(type)(payload)

const processJob = (params, done) =>
  timeout(visibilityTimeout * 1000, handleJob, params)
    .then(nAry(0, done))
    .catch(wrapError(params))
    .catch(done)

const timeout = (delay, f, params) => 
  new Promise((resolve, reject) => {
    setTimeout(() =>
      reject(new Error('visibility timeout exceeded'))
    , delay)

    Promise.resolve(params)
      .then(f)
      .then(resolve)
      .catch(reject)
  })

The only mutable function above is assign, and everything else is nice and functional, and hopefully more readable. And I'm sure I missed a few bits, but that's the gist of it.

[flintinatux]

This function is mutable, and can sometimes be called twice on the same error, which is kinda 👃 .

[flintinatux]

Yeah, I'm not seeing any params anywhere. Do we have linting for this library?