parse-community / parse-server-push-adapter

A push notification adapter for Parse Server
https://parseplatform.org
MIT License
87 stars 99 forks source link

Update apn to version 2.1.2 #52

Closed Schwobaland closed 7 years ago

Schwobaland commented 7 years ago

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 commented 7 years ago

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

Schwobaland commented 7 years ago

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 commented 7 years ago

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

flovilmart commented 7 years ago

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

Schwobaland commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@flovilmart Sounds good! I'll wait for your PR πŸ‘

jeacott1 commented 7 years ago

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 commented 7 years ago

@jeacott1 what do you mean by unstable?

jeacott1 commented 7 years ago

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

flovilmart commented 7 years ago

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

jeacott1 commented 7 years ago

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

flovilmart commented 7 years ago

@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[bot] commented 7 years ago

Codecov Report

:exclamation: 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 commented 7 years ago

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