Update apn to version 2.1.2 #52

Schwobaland · 2017-02-03T07:31:17Z

Update to apn. This enables parse to use http2-API from Apple. Tokens can be used instead of certificates. So one push-config enables to push to different apps.

There are some new options for configuring push-adapter:

@param {Object} args.token {Object} Configuration for Provider Authentication Tokens. (Defaults to: null i.e. fallback to Certificates)
@param {Buffer|String} args.token.key The filename of the provider token key (as supplied by Apple) to load from disk, or a Buffer/String containing the key data.
@param {String} args.token.keyId The ID of the key issued by Apple
@param {String} args.token.teamId ID of the team associated with the provider token key
@param {String} args.topic Specififies an App-Id for this Provider

Old property bundleId is mapped to topic for backward-compatibility.

natanrolnik · 2017-02-03T11:13:02Z

@Schwobaland thanks for your contribution! It seems that the tests are failing. Can you take a look why?

Schwobaland · 2017-02-03T12:55:31Z

Seems that apn 2.1.2 is not compatible with node 4.3. Tests are running with node 5. Parse-server has node 4.3 as minimum requirement. So - should i close this pull request? Or can minimum requirements be changed to node 5 or 6 (lts-version).

flovilmart · 2017-02-06T14:50:51Z

@Schwobaland we bumped the min version to 4.6, and now everything looks good on the tests.

flovilmart · 2017-02-06T14:51:06Z

spec/APNS.spec.js

@@ -1,68 +1,31 @@
-var APNS = require('../src/APNS');
+let APNS = require('../src/APNS');


flovilmart · 2017-02-06T14:51:29Z

spec/APNS.spec.js

-      key: 'prodKey.pem',
+  it('can initialize with cert', (done) => {
+    let args = {
+      cert: new Buffer('testCert'),


this should still work with a string no?

it should work, if the string contains a (resolvable) filename or a string, that matches against /-----BEGIN ([A-Z\s*]+)-----/. See here for details.
Will change one of the test-cases to test this.

flovilmart · 2017-02-06T14:52:24Z

spec/APNS.spec.js

      production: true,
-      bundleId: 'bundleId'
+      topic: 'topic'


I'd keep bundleId as the main option with topic being optional to maximize backwards compatibility.

perhaps if using bundleId we could log a warning: bundleId is deprecated, use topic instead

There was already a mapping of bundleId to topic. I will add a warning, if bundleId is used.

flovilmart · 2017-02-06T14:52:47Z

spec/APNS.spec.js

    expect(prodApnsOptions.cert).toBe(args.cert);
    expect(prodApnsOptions.key).toBe(args.key);
    expect(prodApnsOptions.production).toBe(args.production);
    done();
  });

-  it('can initialize with multiple certs', (done) => {


this is not supported anymore? I personally rely on that capability.

this is supported for different topics/bundleIds.
But I haven't found any possibility, to implement a retry for notifications, that a rejected by apns because of wrong provider or wrong production- / test-device.

Example: You have two provider, one for production use and one for development. Both share the same topic/bundleId. So _chooseProviders would return [prodProvider, devProvider] based on priority-sorting in constructor. So send-Method uses the first one to send the notification. If device is registered using apple-development-provisioning-profile, the notification would not be delivered, because of wrong provider. In parse-device-object is no flag, that describes, wether this device is using development- or production-profile.

In "old" parse-push-adapter (using apn version 1.X), there was a retry-functionality. This isn't available in my version.

I will add this test-case again to test different topics.

+1 for multiple certs

flovilmart · 2017-02-06T14:53:23Z

spec/APNS.spec.js

-    done();
-  });
-
-  it('can choose conns for device with valid appIdentifier', (done) => {


not supported anymore either?

see it('can choose providers for device with valid appIdentifier'....

flovilmart · 2017-02-06T14:55:23Z

src/APNS.js

-    conn.on('timeout', () => {
-      log.verbose(LOG_PREFIX, 'APNS Connection %d Timeout', conn.index);
-    });
+      let provider = this._createProvider(apnsArgs);


the _createProvider should be 'static' as it doesn't need this

flovilmart · 2017-02-06T14:56:02Z

src/APNS.js

+   * @param {String} topic Topic the Notification is sent to
+   * @returns {Object} A apns Notification
+   */
+  _generateNotification(coreData, expirationTime, topic) {


this should stay static as well as is doesn't require this

flovilmart · 2017-02-06T14:57:14Z

src/APNS.js

+      log.verbose(LOG_PREFIX, 'APNS transmitted to %s', token);
+      return this._createSuccesfullPromise(token);
+    });
+    response.failed.forEach((failure) => {


this will just be a no-op as it will create promises that are never handled.

will return a Promise.all with all promises

That's better :)

flovilmart · 2017-02-06T14:58:02Z

Hi @Schwobaland, thanks for that Pull Request! We have a few things here and there before we can merge that one! Thanks again!

…sh-adapter into apn2 * 'apn2' of https://github.com/Schwobaland/parse-server-push-adapter: Update .travis.yml Update package.json

Schwobaland · 2017-02-08T10:15:11Z

Hi @flovilmart, thanks for commenting. Fixed some of your requested changes. Please review this comment. I'm not sure, how to handle this, because of backward-compatibility.

I will be using two instances of parse-server: One for production and one for development. So for my use-cases this is not a problem, but...

flovilmart · 2017-02-08T13:28:15Z

@Schwobaland I'll have a look for multiple certs for a single topic, and I'll open a Pr on your branche before we get to merge that what do you think?

Schwobaland · 2017-02-08T14:28:03Z

@flovilmart Sounds good! I'll wait for your PR 👍

jeacott1 · 2017-02-16T03:10:20Z

src/APNS.js

+    response.failed.forEach((failure) => {
+      if (failure.error) {
+        log.error(LOG_PREFIX, 'APNS error transmitting to device %s with error %s', failure.device, failure.error);
+        promises.push(PNS._createErrorPromise(failure.device, failure.error));


typo. PNS should be APNS

@jeacott1 thanks. Fixed this.

jeacott1 · 2017-02-24T03:39:20Z

I've been playing with apn 2.1.3, and thus far its been pretty unreliable unfortunately. the existing apn 1.x is more stable, but tracking doesn't work well.

flovilmart · 2017-03-05T19:45:54Z

@jeacott1 what do you mean by unstable?

jeacott1 · 2017-03-05T23:09:31Z

http2 errors, and other sporadic exceptions. many have been mitigated by downgrading node to 5.11.1, but that causes other performance issues.

flovilmart · 2017-03-06T00:26:26Z

Anything worth reporting to the APN maintainer? Or related to the current implementation?

jeacott1 · 2017-03-06T01:54:04Z

I think most of the issues are already documented either with apn, http2, or nodejs.
eg: node-apn/node-apn#449

flovilmart · 2017-03-18T19:00:16Z

@Schwobaland seems that it's getting updated to 2.1.3 last month, can you bump? I may takeover your branch as I'm in the midst of removing babel altogether from the package.

codecov · 2017-03-19T20:43:00Z

Codecov Report

❗ No coverage uploaded for pull request base (master@6f382f1). Click here to learn what that means.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             master    #52   +/-   ##
=======================================
  Coverage          ?   100%           
=======================================
  Files             ?      5           
  Lines             ?    249           
  Branches          ?      0           
=======================================
  Hits              ?    249           
  Misses            ?      0           
  Partials          ?      0

Impacted Files	Coverage Δ
src/index.js	`100% <ø> (ø)`
src/APNS.js	`100% <100%> (ø)`
src/GCM.js	`100% <100%> (ø)`
src/ParsePushAdapter.js	`100% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f382f1...f667715. Read the comment docs.

flovilmart · 2017-03-19T20:49:56Z

Closing as superset by #72 , thanks @Schwobaland for getting the ball rolling on that, one!

update to apn version 2

486e45a

Merge branch 'master' into apn2

628631e

flovilmart suggested changes Feb 6, 2017

View reviewed changes

flovilmart mentioned this pull request Feb 8, 2017

apns never reports numFailed in _PushStatus. #55

Closed

natanrolnik mentioned this pull request Feb 8, 2017

Support Apple's APNS Token Authentication #50

Closed

Schwobaland added 2 commits February 8, 2017 11:08

correcting some errors

568ed4d

Merge branch 'apn2' of https://github.com/Schwobaland/parse-server-pu…

e80eef7

…sh-adapter into apn2 * 'apn2' of https://github.com/Schwobaland/parse-server-push-adapter: Update .travis.yml Update package.json

jeacott1 reviewed Feb 16, 2017

View reviewed changes

fix typo

f667715

funkenstrahlen mentioned this pull request Mar 15, 2017

Parse Server Crash on invalid/expired APNS cert parse-community/parse-server#3575

Open

Schwobaland mentioned this pull request Mar 19, 2017

APN with HTTP/2 #72

Merged

flovilmart closed this Mar 19, 2017

hipwelljo mentioned this pull request Sep 28, 2017

Update Parse Server Guide for token based authentication for push notifications parse-community/parse-server#4215

Closed

		@@ -1,68 +1,31 @@
		var APNS = require('../src/APNS');
		let APNS = require('../src/APNS');

Update apn to version 2.1.2 #52

Update apn to version 2.1.2 #52

Conversation

Schwobaland commented Feb 3, 2017

natanrolnik commented Feb 3, 2017

Schwobaland commented Feb 3, 2017

flovilmart commented Feb 6, 2017

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

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

flovilmart commented Feb 6, 2017 • edited Loading

Schwobaland commented Feb 8, 2017

flovilmart commented Feb 8, 2017

Schwobaland commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeacott1 commented Feb 24, 2017

flovilmart commented Mar 5, 2017

jeacott1 commented Mar 5, 2017

flovilmart commented Mar 6, 2017

jeacott1 commented Mar 6, 2017 • edited Loading

flovilmart commented Mar 18, 2017

codecov bot commented Mar 19, 2017 • edited Loading

Codecov Report

flovilmart commented Mar 19, 2017

flovilmart commented Feb 6, 2017 •

edited

Loading

jeacott1 commented Mar 6, 2017 •

edited

Loading

codecov bot commented Mar 19, 2017 •

edited

Loading