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

Notifications not showing up on Windows 10 #43

Open
david-hollifield opened this issue Aug 14, 2019 · 19 comments
Open

Notifications not showing up on Windows 10 #43

david-hollifield opened this issue Aug 14, 2019 · 19 comments

Comments

@david-hollifield
Copy link

After an update to Windows 10 1903, notifications stopped popping up. They do show up as notifications in the notification area after a reboot. Any ideas?

@RoccoC
Copy link
Owner

RoccoC commented Aug 14, 2019

Hi @david-hollifield , thanks for the report! I'm seeing the same thing after upgrading to Windows 10 1903 as well. I will investigate, but may be something that needs to be addressed in node-notifier: mikaelbr/node-notifier#277.

@quintonn
Copy link

quintonn commented Aug 28, 2019

Hi @RoccoC,
I'm experiencing the same issue.
I notice in your call to notifier.notify you set the wait flag to !buildSuccessful.
So that means wait is set to false. On the node-notifier link you posted, wait should be set to true to work around the windows issue.
Is that maybe something you can change in your code, or is there a reason you don't want to set wait to true?

PS. I changed the wait to true on my local version and it now shows the notification

@RoccoC
Copy link
Owner

RoccoC commented Aug 29, 2019

Hi @quintonn, good question. Setting wait to true does fix the issue with missing notifications in Windows 10, but it also prevents the webpack build from completing until the notification is dismissed, which would regress #45.

I suppose we could temporarily set wait to true on Windows machines until the root cause is fixed? Alternatively, we could allow the consumer to override the wait option via config? Thoughts?

@quintonn
Copy link

Hi @RoccoC, I didn't realize it prevents a build until the notification is dismissed.
Would it help calling the notify with setTimeout?
I tested it on my computer and it seemed ok.
I will create a PR and you can decide if you want to keep it.

@quintonn
Copy link

This fix might fix bug 45 also?

@RoccoC
Copy link
Owner

RoccoC commented Sep 11, 2019

@quintonn , just tested out your PR, thanks. 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. :)

@RoccoC
Copy link
Owner

RoccoC commented Sep 12, 2019

@quintonn, I think I've come up with a fix. Would you mind testing using the bugfix/blocking-notifications branch to ensure that 1) notifications appear as expected and 2) the build is no longer blocked when a notification is dispatched?

Note that in the case of error notifications, the build will always be blocked until the notification is dismissed, however.

You can target the branch version by updating the dependency in your project's package.json as follows:

"webpack-build-notifier": "RoccoC/webpack-build-notifier#bugfix/blocking-notifications"

Let me know how it goes, thanks!

@quintonn
Copy link

I've tested this change but i don't see notifications now.
But looking at the index.js file, you've removed the setTimeout on the call to notifier.notify.

And also wait is set to !buildstatus again, which is false.

If i set wait to true, i see the notification but build doesn't complete.
It needs to the timeout feature for this to work i think.

But i am wondering if all of this is worth it, The fix should be in node-notifier.

@quintonn
Copy link

If i put the setTimeout back on the notifier.notify call and with webpack watch, if i make a change, save it, it shows the notification, and without closing the notification, if i make another change and save it, it shows another notification.

I think the issue might be related to the fact that Node only uses 1 thread, so the setTimeout only moves that call to be the last call being executed, so it will always wait for the notification to be dismissed if we set wait to true.

@RoccoC
Copy link
Owner

RoccoC commented Sep 13, 2019

Which version of webpack is your project using?

@quintonn
Copy link

"webpack": "^4.39.2",
"webpack-build-notifier": "RoccoC/webpack-build-notifier#bugfix/blocking-notifications",

I see there is a bit newer version. will try that one

@quintonn
Copy link

On new version it showed the notification on the first build without waiting for the notification to be dismissed, perfect.
But on second and 3rd builds it doesn't show.
Error shows and waits till i dismiss the notification.

Using webpack watch also does not show the notification anymore.

Ps. i'm going on holiday today, so won't be able to help with this for a while

@david-hollifield
Copy link
Author

Maybe updating to the latest node-notifier (with latest SnoreToast) will resolve this issue.

mikaelbr/node-notifier#277 (comment)

@GiancarlosIO
Copy link

👀

@RoccoC RoccoC closed this as completed in 1a5987d Oct 15, 2019
@RoccoC RoccoC reopened this Oct 15, 2019
@RoccoC
Copy link
Owner

RoccoC commented Oct 15, 2019

I've updated to node-notifier@6.0.0. This appears to have fixed the issue. Please let me know if you still have issues after upgrading to webpack-build-notifier@1.2.2.

@RoccoC RoccoC closed this as completed Oct 15, 2019
@david-hollifield
Copy link
Author

@RoccoC Works great! Thank you!

@RoccoC
Copy link
Owner

RoccoC commented Oct 16, 2019

Great, thanks for verifying!

@david-hollifield
Copy link
Author

I'm not sure what's happened, but notifications have stopped working again :(.

@RoccoC
Copy link
Owner

RoccoC commented Dec 19, 2019

@david-hollifield -- apologies for the delay in my response. I will have a look this morning.

@RoccoC RoccoC reopened this Dec 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants