Closed quintonn closed 5 years ago
Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
---|---|---|---|---|---|
index.js | 5 | 6 | 83.33% | ||
<!-- | Total: | 5 | 6 | 83.33% | --> |
Totals | |
---|---|
Change from base Build 49: | 0.3% |
Covered Lines: | 73 |
Relevant Lines: | 112 |
Thanks for the PR. Will review over the next couple of days.
I've merged this into a new branch, bugfix/blocking-notifications
so I can do some manual testing before reintegrating into master
. Will update here shortly with my findings. Thanks!
Hmm, in my testing it seems that while the notification is now shown consistently in Windows (due to setting wait
to true
, the build is still blocked until the notification is dismissed. I wonder if we could instead fork a detached process to dispatch the notification? I'll try and think of other solutions, open to any suggestions as well. :)
How do you go about testing it? I am using webpack in watch mode (-w) and my builds are succeeding as i never dismiss the notification. Maybe i've done something different
If you run without watch mode, you should see that the build completes only once the notification has been dismissed.
It seems you're right. But the notification dismisses itself after a few seconds and then the build is done. but i guess that's not a good solution. Guess we need to wait for node-notifier to fix the issue.
Changed the call to notifier.notify async using setTimeout so it doesn't block the build, and also set wait to false