Remove useless error thrown

[wmcmurray]

Knock Knock 🚪 anybody still here ? 😄

I noticed an error in our logs : You must only call this before calling Notification.requestPermission(), otherwise this feature detect would trigger an actual notification! and I know why it happens.

The problem :

Imagine a scenario where a user opens up a web chat app in two different browser tab. At some point the user receives a chat message and the app ask for permission to display web notifications. Since the app is realtime, it will ask for permission in BOTH tabs at the same time. The user may then click "allow" in the current tab and leave the other untouched.

At this point, Notify.needsPermission will still be true in the other tab even if N.permission === 'granted', which will then lead us to calling Notify.isSupported() and triggering the error in the other tab when a new message is received.

The fix :

I think it's safe to assume that if the permission has been granted already, this feature MUST be supported ! Therefore we can just return true there, instead of throwing and error.

In fact, to completely fix this scenario we would need to refactor Notify.needsPermission into a method/getter that checks if N.permission === 'granted' equals true instead of a boolean initialized on start 🤷‍♂️

[alexgibson]

This seems like a simple code change at first, but the error in place is related to the following issue https://developers.google.com/web/updates/2015/05/Notifying-you-of-notificiation-changes#android_notifications (which can also be found via the link in the comments for the function you're updating).

For the usecase you describe above, your code change makes perfect sense. However, altering it in this way there's now a chance (I think) that Notify.isSupported() could return a false positive of true on Android, should someone first grant permission for a push notification (which is supported), and then error when some code tries to trigger a web notification instead.

Saying that, I think the above scenario is probably less likely than the error you're seeing above in your logs. Looking at Modernizr, they also follow the same assumption in their detection.

It looks like Travis failed with some test failures. If you update those, I think we can likely merge this.

[alexgibson]

In fact, to completely fix this scenario we would need to refactor Notify.needsPermission into a method/getter that checks if N.permission === 'granted' equals true instead of a boolean initialized on start 🤷‍♂️

I think this makes good sense too.

[wmcmurray]

Ah ! good to know. Sadly I couldn't find a more recent workaround to this issue dating from 2015 :/ And I am unsure of the best approach for this... I kinda tend toward maybe putting a try/catch directly into Notify.show() to catch this android error case instead of attempting to send an empty notification in the Notify.isSupported() method, but I'm not sure lol 🤷‍♂️

Anyway, I fixed the tests and implemented Notify.needsPermission as a getter !

I almost had to add a check for IE <= 8 (because no getters support), but since Notifications are not supported as well, I just set Notify.needsPermission = true; in that case, which will redirect the flow toward Notify.isSupported() and it will stop there because Notifications are not supported anyway.

[alexgibson]

I almost had to add a check for IE <= 8 (because no getters support), but since Notifications are not supported as well, I just set Notify.needsPermission = true; in that case, which will redirect the flow toward Notify.isSupported() and it will stop there because Notifications are not supported anyway.

Hmm, burrying away implied logic feels like a code smell to me. All this just to use a getter? Would it be worth making this just a plain old method?

[wmcmurray]

It would be simpler yes, and it would be more consistant with the rest of the API which consists of methods instead of boolean values, but that would be a breaking change 🤷‍♂️

...your call ! :D

[alexgibson]

The next time I publish a release of this library it's going to have some substantial breaking changes anyway, so feel free...