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

messaging: allow calling useServiceWorker() multiple times #2496

Open
nuragic opened this issue Jan 7, 2020 · 5 comments
Open

messaging: allow calling useServiceWorker() multiple times #2496

nuragic opened this issue Jan 7, 2020 · 5 comments

Comments

@nuragic
Copy link

nuragic commented Jan 7, 2020

Quoting @gauntface in #114 (comment)

The error message does need changing. I hadn't intended that developers would call this method twice, out of interest what is the use case?

The use case is e.g. having a toggle to enable/disable push notifications.

The reason for enforcing a single call is to avoid any risk of the library being called with two different service workers.

Ok, good thing! However, I know what I'm doing in this case, so I'd like to have a mechanism to circumvent the registrationToUse != null check.

So would people prefer the ability to just allow any number of calls and block after getToken is called or would they prefer only being able to call it once and just have the error corrected to something along the lines of "useServiceWorker can only be called once and must be called before getToken()" ?

As I said, I need the possibility to call this method N times during the same app lifecycle...

Imagine this scenario:

  • the app loads from scratch, the SW is registered... the user previously enabled push notifications, so now we call 1) useServiceWorker() 2) getToken() .... etc.
  • user disable notifications from the app UI, we call deleteToken()... cool
  • user wants to enable notifications again: here we do the same thing as when the app initialise, but we get this error...

The fix could be either to remove the check and not throw, to expose a setter for registrationToUse in order to be reset, or passing a new flag to useServiceWorker() in order to enable multiple calls.

Thanks! 🙏

@google-oss-bot
Copy link
Contributor

I found a few problems with this issue:

  • I couldn't figure out how to label this issue, so I've labeled it for a human to triage. Hang tight.
  • This issue does not seem to follow the issue template. Make sure you provide all the required information.

@mmermerkaya
Copy link
Contributor

I don't see why you'd need to call useServiceWorker() for a second time in your example. Are you unregistering the service worker when you call deleteToken()?

@nuragic
Copy link
Author

nuragic commented Jan 9, 2020

No, I'm not unregistering. I just have a toggle on the UI to enable/disable the push notifications.

Why I have to call it more than once? Because if not, my logic would need some flag to discriminate if I called this function once, or more times... and I do not need any flag... I just need my logic to work the first, second, and Nth case, the same way, without having to introduce additional logic...

Like it is possible to call await Notification.requestPermission() multiple times (which I call on my "enable" logic), it should be possible to do so with this function.

My enable function looks like this:

enablePush = async () => {
    const permission = await Notification.requestPermission();
    if (permission === 'granted') {
      const registration = await navigator.serviceWorker.getRegistration();
      // throws here when you enable for the second time
      await messaging.useServiceWorker(registration);
      const token = await this.messaging.getToken();
      if (token != null) {
        messaging.onTokenRefresh(myOnRefreshFn);
        mySavePushTokenFn(token);
      }
      // ... 
    }
  };

I hope that you can see that now...

What I'm doing to avoid the error right now? I just added this line before the useServiceWorker call:

messaging.registrationToUse = null;

This is an hack because that prop is marked as private in the source, it may not work in the future... I think a better way could be provided.

What would you do?

@nuragic
Copy link
Author

nuragic commented Jan 9, 2020

In case it would be useful, here is the disable logic: 😄

disablePush = async () => {
  theStopRefreshFnRetunedByInitRefresh();
  const token = await messaging.getToken();
  await messaging.deleteToken(token);
  myDeletePushTokenFn(token);
};

@nuragic
Copy link
Author

nuragic commented Jan 9, 2020

For the record, here is the callback that executes when my SW registers (is called every time the app starts from scratch):

const onSWRegister = (registration) => {
  const hasPushSubscription = registration.pushManager
    ? await registration.pushManager.getSubscription()
    : false;

  if (hasPushSubscription) {
    // cool, user has push enabled:    
    // basically the same logic as in the `enable` function:

    // this never throws, because it could be only the first time
    messaging.useServiceWorker(registration);

    const token = await this.messaging.getToken();
    if (token != null) {
      messaging.onTokenRefresh(myOnRefreshFn);
      mySavePushTokenFn(token);
    }
  }
},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants