COMPASS-375: Tell the user when an unexpected error occurs #702
Conversation
| @@ -7,6 +7,8 @@ var format = require('util').format; | |||
| var ipc = require('hadron-ipc'); | |||
| var intercom = require('./intercom'); | |||
| var features = require('./features'); | |||
| var Notifier = require('node-notifier'); | |||
| var process = require('process'); | |||
There was a problem hiding this comment.
You shouldn't need to require process here - it is a global variable.
| @@ -94,6 +96,19 @@ module.exports = function() { | |||
|
|
|||
| window.addEventListener('error', function(err) { | |||
| debug('error encountered, notify trackers', err); | |||
| // Notify user that error occurred | |||
| if (!_.includes(err.message, 'MongoError')) { | |||
| var icon = pkg.config.hadron.build.win32.icon; | |||
There was a problem hiding this comment.
If you're intent on being able to change the reference to this variable, I would use ES6 syntax instead and use let icon instead of var icon. However, my personal preference would be to treat it as a const and use a ternary operator to set the value, like:
const icon = (process.platform === 'darwin') ?
pkg.config.hadron.build.darwin.icon : pkg.config.hadron.build.win32.iconThere was a problem hiding this comment.
ok, I can change it to a ternary statement. Is this a style issue or does it have other effects?
| @@ -94,6 +96,19 @@ module.exports = function() { | |||
|
|
|||
| window.addEventListener('error', function(err) { | |||
| debug('error encountered, notify trackers', err); | |||
| // Notify user that error occurred | |||
| if (!_.includes(err.message, 'MongoError')) { | |||
There was a problem hiding this comment.
I wouldn't check the message to determine if the error is a MongoError - I would use the name attribute, as all MongoErrors set the name attribute to MongoError: https://github.com/christkv/mongodb-core/blob/2.0/lib/error.js#L11
There was a problem hiding this comment.
MongoErrors do have that name but the error object passed to the function isn't the error itself, it's been modified to look like:
There was a problem hiding this comment.
Ah ok sorry... Thought it was the actual error. :)
|
LGTM |
imlucas
changed the title
more details›
Developer Notes
The only slightly iffy part about this is that the notification has a terminal icon as well as the compass icon, which is a known issue with node-notifier.