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

add console.error for async TranscriptLogger.logActivity() impl in TranscriptLoggerMiddleware #669

Merged
merged 3 commits into from
Dec 5, 2018

Conversation

stevengum
Copy link
Member

Fixes #383

Description

Previously, if asynchronous TranscriptLogger.logActivity() implementations threw an error in TranscriptLgogerMiddleware, an UnhandledPromiseRejectionWarning would occur. This PR changes TranscriptLoggerMiddleware to instead console.error the error thrown via an appended catch().

Specific Changes

  • Add logic to append catch if TranscriptLoggerMiddleware.logger.logActivity() returns a Promise
  • Add tests for synchronous and asynchronous error handling in TranscriptLoggerMiddleware.onTurn() when calling this.logger.logActivity()

@stevengum stevengum added the 4.2 December 15, 2018 Release label Dec 2, 2018
@coveralls
Copy link

coveralls commented Dec 2, 2018

Pull Request Test Coverage Report for Build 2391

  • 9 of 9 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 82.326%

Totals Coverage Status
Change from base Build 2386: 0.04%
Covered Lines: 2671
Relevant Lines: 3122

💛 - Coveralls

Copy link
Member

@johnataylor johnataylor left a comment

Choose a reason for hiding this comment

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

just one comment - for consideration

// tslint:disable-next-line:no-console
console.error('TranscriptLoggerMiddleware logActivity failed', err);
});
}
} catch (err) {
Copy link
Member

Choose a reason for hiding this comment

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

This is something of a nit.

If this err object is of type Error (which it should be but isn't necessarily) then this line will do a toString which will write only a subset of the information being carried to the console.

Wouldn't it therefore be more correct to explicitly check if it was of type Error and if it was then print out more of the fields, otherwise just toString()? (Incidentally, if I remember correctly I don't think JSON.stringify will work on Error.)

In this particular case I'm not sure this matters to much because its just the console. But something to consider in general.

Copy link
Member Author

Choose a reason for hiding this comment

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

How about this?

try {
  const activity: Activity = this.transcript.shift();
  // Comments comments comments...
  const logActivityResult = this.logger.logActivity(activity);
  // Comments comments comments...

  if (logActivityResult.catch(err => {
    this.transcriptLoggerErrorHandler(err);
  });
} catch (err) {
  this.transcriptLoggerErrorHandler(err);
}

private transcriptLoggerErrorHandler(err: Error|Any): void {
  // tslint:disable:no-console
  if (err instanceof Error) {
    console.error(`TranscriptLoggerMiddleware logActivity failed: "${ err.message }"`);
    console.error(err.stack);
  } else {
    console.error(`TranscriptLoggerMiddleware logActivity failed: "${ JSON.stringify(err) }"`);
  }
  // tslint:enable:no-console
}

Copy link
Member

Choose a reason for hiding this comment

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

very nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.2 December 15, 2018 Release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants