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

Added a proper catch block for handling errors #385

Merged
merged 3 commits into from Jan 31, 2020

Conversation

gnought
Copy link
Collaborator

@gnought gnought commented Jan 31, 2020

@mcollina Is it better to add .catch() block rather than try/catch?
any differences?

@gnought gnought requested a review from mcollina January 31, 2020 11:45
@mcollina
Copy link
Collaborator

the try catch will save some promise allocation. However there is a risk that the catch part in the try/catch throw, resulting in an unhandled rejection.

This code is fine, it's just for tests.

@gnought
Copy link
Collaborator Author

gnought commented Jan 31, 2020

Thanks for the explanation. Could we stick into a practise and merge it into the master branch?

test/connect.js Outdated
})
} catch (error) {
await Promise.all([msg(s, 100, packet1), msg(s, 200, packet2)])
setImmediate(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would make this a promise and use await.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  function finish () {
    return new Promise((resolve, reject) => {
      broker.close()
      resolve(t.end)
    })
  }
  (async () => {
    await Promise.all([msg(s, 100, packet1), msg(s, 200, packet2)])
    await finish()
  })().catch(
    (error) => {
      t.fail(error)
    }
  )

Is the above what you mean? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or should it be like this?

  (async () => {
    await Promise.all([msg(s, 100, packet1), msg(s, 200, packet2)])
  })().catch(
    (error) => {
      t.fail(error)
    }
  ).finally(finish)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean

const immediate = promisify(setImmediate)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

awesome!. I haven't seen we could promisify setImmediate.
Like this?

  const immediate = util.promisify(setImmediate)

  ;(async () => {
    await Promise.all([msg(s, 100, packet1), msg(s, 200, packet2)])
    await immediate().then(() => {
      broker.close()
      t.end()
    })
  })().catch(
    (error) => {
      t.fail(error)
    }
  )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I would make this a promise and use await.

May we know what the differences are?

test/connect.js Outdated
await Promise.all([msg(s, 100, packet1), msg(s, 200, packet2)])
setImmediate(() => {
await immediate().then(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
await immediate().then(() => {
await immediate()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

awesome. You are a great teacher!

Copy link
Collaborator

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@gnought gnought merged commit 0204c8f into moscajs:master Jan 31, 2020
@gnought gnought deleted the hotfix/add_catch_block branch January 31, 2020 17:28
@github-actions
Copy link

Pull Request Test Coverage Report for Build 16556a6f9c59eee72db902f6900eaad9d2fdd6d1-PR-385

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.08%) to 95.802%

Totals Coverage Status
Change from base Build 3e0af1fca5dc2b606acea4e8a2faf10d05e01970: 0.08%
Covered Lines: 821
Relevant Lines: 842

💛 - Coveralls

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.

None yet

2 participants