Skip to content

Avoid swallowing errors in .after() without argument#74

Closed
mcollina wants to merge 1 commit intomasterfrom
fix-after-error-swallow
Closed

Avoid swallowing errors in .after() without argument#74
mcollina wants to merge 1 commit intomasterfrom
fix-after-error-swallow

Conversation

@mcollina
Copy link
Copy Markdown
Member

@mcollina mcollina commented Sep 3, 2018

This is going to be contentious about the semversiness.

Without this change, the following fails:

  const fastify = fastify()
  const payload = { hello: 'world' }

  fastify.register((instance, opts) => {
    return promise.reject(new error('kaboom'))
  }).after(function () {
    t.pass('after is called')
  })

  fastify.inject({
    method: 'get',
    url: '/'
  }).then(() => {
    t.fail('this should not be called')
  }).catch(err => {
    t.ok(err)
    t.strictequal(err.message, 'kaboom')
  })

Note that this could cause several horrible bugs like having 404 with routes not defined because an error got swallowed.

Copy link
Copy Markdown
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Sep 4, 2018

that is not affected, as it has 2 arguments.

Copy link
Copy Markdown
Member

@delvedor delvedor left a comment

Choose a reason for hiding this comment

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

LGTM

@mcollina
Copy link
Copy Markdown
Member Author

mcollina commented Sep 4, 2018

Included in #75.

@mcollina mcollina closed this Sep 4, 2018
@mcollina mcollina deleted the fix-after-error-swallow branch September 4, 2018 07:48
mcollina added a commit to fastify/fastify that referenced this pull request Sep 4, 2018
mcollina added a commit to fastify/fastify that referenced this pull request Sep 4, 2018
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