New send notification #218

gauntface · 2016-09-18T01:17:28Z

@marco-c sorry that this is such a large set of changes.

Found a couple of issues in the sendNotification tests, I've re-organised the code a little to be easier to separate concerns a bit more and added a few simple error checks to catch errors earlier on.

# The first commit's message is: Changes to new API surface for sendNotification # This is the 2nd commit message: Add more files to .npmignore

marco-c · 2016-09-18T01:19:39Z

bin/web-push.js

@@ -63,4 +66,7 @@ webPush.sendNotification(endpoint, params).then(() => {
 }).then(() => {
  process.exit(0);
 });
+<<<<<<< HEAD


Leftover from merge.

marco-c · 2016-09-18T01:20:04Z

src/encryption-helper.js

+const ece = require('http_ece');
+const urlBase64 = require('urlsafe-base64');
+
+// New standard, Firefox 46+ and Chrome 50+.


Nit: I guess we could remove this comment now.

gauntface · 2016-09-18T04:05:47Z

@marco-c good news, got FF running on Linux and OS X without the GH_TOKEN.

marco-c · 2016-09-19T21:14:39Z

src/vapid-helper.js

+
+/**
+ * This method takes the required VAPID parameters and returns the required
+ * header to added to a Web Push Protocol Request.


Nit: to be added

marco-c · 2016-09-19T21:14:46Z

src/vapid-helper.js

+/**
+ * This method takes the required VAPID parameters and returns the required
+ * header to added to a Web Push Protocol Request.
+ * @param  {string} audience       This must be the origni of the push service.


Nit: origin

marco-c · 2016-09-19T21:17:12Z

src/web-push-lib.js

+      }
+    }
+
+    // Test subscription is an Object


Are these comments notes for tests that you wanted to write?

marco-c · 2016-09-19T21:19:27Z

src/web-push-lib.js

+        requestOptions.headers.Authorization = 'key=' + currentGCMAPIKey;
+      }
+    } else if (currentVapidDetails) {
+      // VAPID isn't supported by GCM.


Nit: This comment no longer applies here, should be moved to the if before

marco-c · 2016-09-19T21:20:58Z

test/test-vapid-helper.js

+  });
+
+  // TODO:2 good tests for the vapid headers
+  // TODO: Make sure you can v


Nit: this TODO comment is cut 😄

marco-c · 2016-09-19T21:26:18Z

test/testSelenium.js

@@ -31,6 +33,13 @@
  };
  const testDirectory = './test/output/';

+  webPush.setGCMAPIKey('AIzaSyAwmdX6KKd4hPfIcGU2SOfj9vuRDW6u-wo');
+  webPush.setVapidDetails(


This means that we're going to run all tests with vapid, right? Unless you change tests like:

test('send/receive notification with payload with ' + browser.getPrettyName(), function() { this.timeout(PUSH_TEST_TIMEOUT); return runTest(browser, { payload: 'marco' }); });

to:

test('send/receive notification with payload with ' + browser.getPrettyName(), function() { this.timeout(PUSH_TEST_TIMEOUT); return runTest(browser, { payload: 'marco', vapid: null, }); });

marco-c · 2016-09-19T21:26:42Z

test/testSelenium.js

@@ -163,18 +177,15 @@
        }

        if (!pushPayload) {
-          promise = webPush.sendNotification(subscription.endpoint, {
+          promise = webPush.sendNotification(subscription, null, {
            vapid: vapid


This should be vapidDetails

Argh. This is exactly the kind of problem I'm trying to avoid :P

I've added an extra check and test to avoid unexpected keys in the extra options.

marco-c · 2016-09-19T21:28:16Z

test/testSetGCMAPIKey.js

@@ -25,4 +31,10 @@ suite('setGCMAPIKey', function() {
      webPush.setGCMAPIKey(42);
    }, Error);
  });
+
+  test('undefined valud', function() {


marco-c · 2016-09-19T21:28:38Z

@marco-c good news, got FF running on Linux and OS X without the GH_TOKEN.

Great! Are you opening a new PR for that? Or do you want to update this one?

marco-c · 2016-09-19T21:29:26Z

LGTM, just a bunch of nits and a comment regarding the Selenium tests (with the changes we're going to run everything with VAPID, but before we were testing both with and without VAPID).

marco-c · 2016-09-20T00:18:24Z

src/encryption-helper.js

  }

  if (!userAuth) {
    throw new Error('No user auth provided for encryption.');
  }

  if (typeof userAuth !== 'string') {
-    throw new Error('User auth must be a string.');
+    throw new Error('The subscirption auth key must be a string.');


Nit: subscription :)

marco-c · 2016-09-20T00:20:21Z

Oh, we also need to fix the documentation in the README for the new signature.

gauntface

I've addressed all the comments you've raised, thank you so much for the review.

I've changed back to the old way of managing vapid details and that has actually fixed an issue I was seeing on my OS X machine but no where else.

Regarding the README, do you want to update that now or just before we publish this signature?

I expect many developers are using the README for the current API surface.

gauntface · 2016-09-20T01:13:48Z

test/testSelenium.js

@@ -163,18 +177,15 @@
        }

        if (!pushPayload) {
-          promise = webPush.sendNotification(subscription.endpoint, {
+          promise = webPush.sendNotification(subscription, null, {
            vapid: vapid


Argh. This is exactly the kind of problem I'm trying to avoid :P

I've added an extra check and test to avoid unexpected keys in the extra options.

gauntface · 2016-09-20T01:35:28Z

Before changing README and release new version, worth getting #210 and #213 in.

marco-c · 2016-09-20T12:48:29Z

Regarding the README, do you want to update that now or just before we publish this signature?

I expect many developers are using the README for the current API surface.

Before changing README and release new version, worth getting #210 and #213 in.

Good idea, let's change it later.

marco-c and others added 3 commits September 17, 2016 18:12

# This is a combination of 2 commits.

369e6e6

# The first commit's message is: Changes to new API surface for sendNotification # This is the 2nd commit message: Add more files to .npmignore

Changes to new API surface for sendNotification

3721969

Merging

1b32864

marco-c reviewed Sep 18, 2016

View reviewed changes

Removing merge conflict markers

373da17

marco-c reviewed Sep 18, 2016

View reviewed changes

Matt Gaunt added 5 commits September 17, 2016 18:29

Removing comment

d31ccf0

Turning off FF tests on OS X on Travis. Geckodriver issue.

58195db

Error out when downloads fail

680bb79

Detect when geckodriver may not be available

9ebc7a1

Try out new geckodriver

5be867e

marco-c reviewed Sep 19, 2016

View reviewed changes

src/web-push-lib.js

}

}

// Test subscription is an Object

Copy link

Member

marco-c Sep 19, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these comments notes for tests that you wanted to write?

marco-c reviewed Sep 19, 2016

View reviewed changes

Removing comments and adding few things to tests

826d6d8

marco-c reviewed Sep 20, 2016

View reviewed changes

gauntface commented Sep 20, 2016

View reviewed changes

Last set of patches from review

8e11cdd

Silly typo :(

3d74337

marco-c merged commit 2fcd717 into web-push-libs:master Sep 20, 2016

marco-c mentioned this pull request Sep 20, 2016

Update README.md #214

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New send notification #218

New send notification #218

gauntface commented Sep 18, 2016

marco-c Sep 18, 2016

gauntface Sep 18, 2016

marco-c Sep 18, 2016

gauntface Sep 18, 2016

gauntface commented Sep 18, 2016

marco-c Sep 19, 2016

marco-c Sep 19, 2016

marco-c Sep 19, 2016

marco-c Sep 19, 2016

marco-c Sep 19, 2016

marco-c Sep 19, 2016

marco-c Sep 19, 2016

gauntface Sep 20, 2016

marco-c Sep 19, 2016

marco-c commented Sep 19, 2016

marco-c commented Sep 19, 2016

marco-c Sep 20, 2016

marco-c commented Sep 20, 2016

gauntface left a comment

gauntface Sep 20, 2016

gauntface commented Sep 20, 2016

marco-c commented Sep 20, 2016

New send notification #218

New send notification #218

Conversation

gauntface commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauntface commented Sep 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marco-c commented Sep 19, 2016

marco-c commented Sep 19, 2016

Choose a reason for hiding this comment

marco-c commented Sep 20, 2016

gauntface left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gauntface commented Sep 20, 2016

marco-c commented Sep 20, 2016