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

Change signature of sendNotification? #192

Closed
gauntface opened this issue Jul 23, 2016 · 4 comments
Closed

Change signature of sendNotification? #192

gauntface opened this issue Jul 23, 2016 · 4 comments
Assignees

Comments

@gauntface
Copy link

gauntface commented Jul 23, 2016

At the moment the signature of sendNotification results in code along
the lines of:

const webpush = require('web-push');

webpush.setGCMAPIKey(GCM_API_KEY);

const subscriptions = getSubscriptions();
subscriptions.forEach(subscription => {
  const params = {
      payload: 'some payload',
      vapid: {
        subject: 'mailto: [email protected]',
        publicKey: VAPID_KEYS.publicKey,
        privateKey: VAPID_KEYS.privateKey
      }
  };

  if (subscription.keys) {
      params.userAuth = subscription.keys.auth;
      params.userPublicKey = subscription.keys.p256dh;
  }

  webpush.sendNotification(subscription.endpoint, params);
});

This could be simplfied to:

const webpush = require('web-push');

webpush.setGCMAPIKey(GCM_API_KEY);
webpush.setVapidDetails({
    subject: 'mailto: [email protected]',
    publicKey: VAPID_KEYS.publicKey,
    privateKey: VAPID_KEYS.privateKey
})

const subscriptions = getSubscriptsion();
subscriptions.forEach(subscription => {
  webpush.sendNotification(subscription, 'some payload');
});

If we wanted to, we could move VAPID and GCM to be optional params
, as requested in #181:

const webpush = require('web-push');

const subscriptions = getSubscriptsion();
subscriptions.forEach(subscription => {
  const options = {
    gcmAPIKey: getGCMAPIKey(subscription),
    vapid: getVAPIDDetails(subscription)
  }
  webpush.sendNotification(subscription, 'some payload', options);
});

What do you think @marco-c @nikhil32?

@marco-c
Copy link
Member

marco-c commented Jul 23, 2016

Both options look good to me. Maybe the first would be a smaller change compared to the second (it wouldn't change the signature for people who aren't using VAPID yet, which is the majority).

@gauntface
Copy link
Author

I was thinking both would be implemented but it would be up to the developer how to pass in GCM and VAPID - Matching PR #193.

Would you be ok with me putting together this change as a PR?

It would be a breaking change so may do a major release at a later stage, but I think it would be a good and worthwhile change.

@marco-c
Copy link
Member

marco-c commented Jul 23, 2016

I was thinking both would be implemented but it would be up to the developer how to pass in GCM and VAPID - Matching PR #193.

Would you be ok with me putting together this change as a PR?

OK, makes sense!

It would be a breaking change so may do a major release at a later stage, but I think it would be a good and worthwhile change.

Yes, I was thinking the same. It won't be a breaking change for most users though (as most users are using GCM or Web Push without VAPID), so the earlier we make the change the better it is.

@nikhil32
Copy link

@gauntface @marco-c thanks for the feature...

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

No branches or pull requests

3 participants