Adding ability to send error object to notify() #2955
Conversation
|
BTW, I wanted to add a test for the modifications, but I was not quite clear on asserting calls to the console. If someone can provide me some guidance, I would be more than happy to add theses tests. |
app/notify.js
Outdated
| @@ -27,9 +27,13 @@ app.on('ready', () => { | |||
| }); | |||
| }); | |||
|
|
|||
| function notify(title, body) { | |||
| function notify(title, body, error) { | |||
There was a problem hiding this comment.
I think it would be a good idea to do {error} as an object on the end, so if we need to send more info to notify in the future we don't need to have another positional argument 👍
There was a problem hiding this comment.
It's funny you mention that, because that was actually my first thought!
I had debated about creating 'channels' represented as properties on the object.
{}.error --> console.error
{}.warn --> console.warn
etc ...
|
@albinekb I modified the parameter as you suggested. Are these changes what you were thinking? |
|
Excellent, thank you so much! |
|
I created a corresponding PR in hyper-site, which updates the API description for 'notify'. |
|
Thank you for your valuable help 🙏 |
In short, this pull request is to assist with error reporting, and diagnostics.
After the 2.0 upgrade, a number of plugins I use stopped working. Unfortunately, while I would see the notification message to check 'Developer tools', there was no further output or logs available.
After looking through the source, I found that some catch blocks would output the error to console.error, but others would not. Rather than modifying all the catch blocks, I opted to add an additional parameter 'error', to the notify method. If present, the error is logged to the console. I then just modified the catch block to pass the error reference.
Please do not be bashful with your feedback, and thank you so much for this incredible tool.