implement notification action

[lxxxvi]

Hi 👋

This is an idea resp. attempt to implement #4

# initial signature from the issue:
turbo_stream.notification(title, options, **attributes)

However... instead of having one options argument, I took the approach of making the options available individually... I am unsure how useful/usless this is. What do you think?

Also, there's one skipped test at the moment.
If we use the actions and/or silent option, the browser log mentions this:

 🚧 Browser logs:
      TypeError: Failed to construct 'Notification': Actions are only supported for persistent notifications shown using ServiceWorkerRegistration.showNotification().
        at Function.invoke (node_modules/sinon/pkg/sinon-esm.js:2386:27)
        at new Notification (node_modules/sinon/pkg/sinon-esm.js:2705:26)
        at StreamElement.rt (dist/index.js:1:12153)
        at Object.renderElement [as render] (node_modules/@hotwired/turbo/dist/turbo.es2017-esm.js:3656:26)
        at node_modules/@hotwired/turbo/dist/turbo.es2017-esm.js:3675:36
        at async StreamElement.connectedCallback (node_modules/@hotwired/turbo/dist/turbo.es2017-esm.js:3660:13)

Do you have an idea how to resolve this, if at all?

[marcoroth]

Thank you @lxxxvi for taking this one on!

However... instead of having one options argument, I took the approach of making the options available individually... I am unsure how useful/usless this is. What do you think?

Technically there are two "signatures", the one shown in the initial issue is the one which could be used from the Ruby side when calling the notification action on turbo_stream. However this doesn't mean that the options also need to be in an options attribute on the <turbo-stream> element itself.

I think we should keep the signature on the Ruby side as close as possible to the regular Notification API, as shown here: https://developer.mozilla.org/en-US/docs/Web/API/Notification/Notification#syntax

Meaning, that if you had a call like:

turbo_stream.notification("My Notification", { lang: "en", body: "The body", icon: "http://url.to.icon" })

it could still get translated into this, using individual attributes for each option:

<turbo-stream title="My Notification" lang="en" body="The Body" icon="http://url.to.icon"></turbo-stream>

But since that is the "responsibility" of the turbo_power-rails repo, we can ignore that for now.

TL;DR: I think it's fine to split up the attributes on the <turbo-stream> element like you did 👍🏼

---

Do you have an idea how to resolve this, if at all?

I would assume that if you use a fake instead of a spy for the test that it could resolve this. Technically the action doesn't need to test if the action actually can instantiate a Notification which it does with the spy. It should be sufficient if we can ensure that the action translates the options in the right new Notification(...) call.

[marcoroth]

I also just realized, we might need to call Notification.requestPermission() first somewhere, otherwise it might fail.

[lxxxvi]

@marcoroth

Thank you 👍

I think I was able to resolve the ServiceWorkerRegistration issue by using stub instead of spy.

[lxxxvi]

I also just realized, we might need to call Notification.requestPermission() first somewhere, otherwise it might fail.

Oh... where should we do that? In the notification function itself?

[lxxxvi]

@marcoroth Thanks a lot for your help today. 🙌

I have refactored the code as discussed. Feel free to review it and let me know if I can improve anything.

[marcoroth]

Thank you @lxxxvi! I think this is pretty good now. I really like how the tests turned out, they are much more readable and easier to understand than what we had before!

I've got one small remark, but other than that this looks really good to me!