Add support for apns-push-type #127

funkenstrahlen · 2019-06-09T15:09:57Z

Required when delivering notifications to devices running iOS 13 and later, or watchOS 6 and later.

Apple docs: https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/sending_notification_requests_to_apns

I opened a PR on node-apn to support this key. Support from node-apn is required to make this PR work.

PR: node-apn/node-apn#656

Required when delivering notifications to devices running iOS 13 and later, or watchOS 6 and later.

codecov · 2019-06-09T15:11:14Z

Codecov Report

Merging #127 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #127   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           5      5           
  Lines         262    266    +4     
=====================================
+ Hits          262    266    +4

Impacted Files	Coverage Δ
src/APNS.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 e19028a...6421e73. Read the comment docs.

funkenstrahlen · 2019-06-09T15:14:46Z

Is node-apn still alive? No commits to master since one year? 😕

funkenstrahlen · 2019-06-20T19:57:37Z

Good news! Node apn package just merged my PR to support this key. I suppose we have to wait for a new release version of the package, link it in dependencies and the this PR is good to be merged.

I started to work on this ASAP because it is required in September when iOS 13 is being released.

dplewis · 2019-06-20T20:04:11Z

@funkenstrahlen They haven't done a releases in a long time, We do have a fork https://github.com/parse-community/node-apn and could use that in the mean time.

funkenstrahlen · 2019-06-21T06:53:32Z

@dplewis Great! So you suggest I submit the same PR to your fork as well?

funkenstrahlen · 2019-06-21T10:05:00Z

I opened a PR on the fork you referenced. I noticed the package currently references the node-apn version 3 tag. As I opened the PR on master it will be required to cleanup the branch structure of the fork to allow using it here in package.json.

funkenstrahlen · 2019-06-27T18:50:20Z

What do you think about setting a default value for apns-push-type key? Because if no value is set a notification will not be delivered to devices running iOS 13, watchOS 6 or later. I suggest setting a default to alert as this is the most common option when calling this API?

If you agree I am happy to add this change to the PR.

dplewis · 2019-07-11T23:21:34Z

@acinader Thoughts?

acinader · 2019-07-11T23:34:28Z

Sorry, @dplewis I don't have an opinion at this point and am unsure of the implications. Maybe @davimacedo can help, or use your best judgment and we'll work out any issues that arise.

davimacedo · 2019-07-11T23:43:10Z

"(Required when delivering notifications to devices running iOS 13 and later, or watchOS 6 and later. Ignored on earlier system versions.) The type of the notification. The value of this header is alert or background. Specify alert when the delivery of your notification displays an alert, plays a sound, or badges your app's icon. Specify background for silent notifications that do not interact with the user."

I think that "alert" by default makes sense since it is the most common use case. We could maybe have a setting in the adapter to change this default to "background" if wanted?

funkenstrahlen · 2019-07-12T07:41:03Z

We could maybe have a setting in the adapter to change this default to "background" if wanted?

I do not think a setting to change the default is necessary because the push type can be set directly when creating a push notification.

The default is only relevant for users who are not aware of the new push type key required for iOS 13.

If we do not define a default push notifications will not be delivered for users on iOS 13 if the developer does not update its server code to define the push type key for each notification explicitly.
If we define a default and the developer is not aware of the change some background notifications might not be delivered correctly because they are marked as alert by default.

davimacedo · 2019-07-12T18:24:39Z

Nice. So let's go ahead with your proposed solution. Could you please resolve the conflicting files?

funkenstrahlen · 2019-07-12T20:39:25Z

I will update the PR this weekend with the changes and also resolve merge conflicts.

If defined explicitly the passed value for push type is used.

funkenstrahlen · 2019-07-13T09:31:40Z

Resolved merge conflicts
Set push_type to alert if not defined explicitly and added a test case for that

To be considered before merging: An updated version of node-apn package is required to support the push_type header key.

funkenstrahlen · 2019-07-13T13:59:03Z

I also created a PR for documenting this change: parse-community/docs#639

davimacedo · 2019-07-14T03:41:29Z

The code seems good to me, but, before merging, we need to solve the problem of node-apn. They haven't launched a new version yet. A see two options:

Change the dependency to their master
Merge the latests commits to our fork and change the dependency to our fork

What do you think?

funkenstrahlen · 2019-07-14T10:12:47Z

It took me quite some effort to get the PR with support for the new header key merged into the node-apn repository. I even had to get in touch via Twitter. I feel like the repository is not under active development any more and we can not expect to see a new release there soon.

Pointing to master (or any other branch) is an option but I strongly advise against that. This removes any control on our side about what state of code gets pulled in via npm. You can do this locally in your own projects if you know what your are doing but not in a project like this where this gets pulled in as a sub dependency.

Therefore I think its best to maintain our own fork. I already created a PR with the required changes for the new push_type header key some time ago. However pointing the apn dependency to our fork also means we need to keep an eye on it in the future to merge any required changes from upstream (if they ever happen).

mman · 2019-07-14T10:25:09Z

+1 on forking, I’m on open source parse since the very beginning and node-apn looks pretty much abandoned since the very beginning of parse push adapter...

davimacedo · 2019-07-14T16:55:44Z

I also prefer to go with our fork. @dplewis @acinader I don't have access to https://github.com/parse-community/node-apn . Could you please accept parse-community/node-apn#1 ? @funkenstrahlen could you also change our dependency to pin our fork in this PR?

funkenstrahlen · 2019-07-14T18:04:54Z

As soon as our fork is updated with the changes and has a new release I will update this PR to use the fork.

davimacedo · 2019-07-25T01:43:44Z

@funkenstrahlen your PR is now merged in our fork. Can you please update this PR and pin to the new release https://github.com/parse-community/node-apn/releases/tag/v2.1.6-parse?

funkenstrahlen · 2019-07-25T07:48:02Z

@davimacedo Sure! I updated the PR with the required changes.

I recognized that we referenced ^3.0.0-alpha1 of node-apn before. The new fork only offers code based on version 2. Do you think this is an issue? The diff is not very big but it could cause some regressions?

funkenstrahlen · 2019-07-25T08:15:48Z

I think after this is merged we should also prepare a new release for this repository. I did not want to mix this into this PR.

davimacedo · 2019-07-25T18:33:08Z

@funkenstrahlen I think you are right. I am thinking to update our fork's master to be pretty much the same they currently have in their master and release again. Do you agree?

funkenstrahlen · 2019-07-25T19:48:36Z

I am thinking to update our fork's master to be pretty much the same they currently have in their master and release again. Do you agree?

I am not sure what's the best approach. 3.0.0-alpha1 tags a commit on the native-http2 branch in the node-apn repository instead of the master. This branch has not been merged with their master for quite some time.

I run parse with node-apn in version 3.0.0-alpha1 on my server for quite some time without any major known issues but this is only my personal experience.

We could start by setting "our" master to their 3.0.0-alpha1 as this has been referenced as a dependency for parse-push-adapter for some time and then just patch this with the apns-push-key changes. This would reduce the chance of surprising braking changes.

In the long run we should take some time and pick some changes from their master and evaluate them to get back on track for the future.

davimacedo · 2019-07-25T20:31:55Z

I agree. Now we have our master synced with the last commit of 3.0.0-alpha1. I hope no one (specially @acinader and @dplewis) will kill me because of the push --force I did 😄 . Just in case, I saved our previous master in a branch called oldmaster. Do you mind to open a new PR with the apns-push-key changes?

funkenstrahlen · 2019-07-26T07:49:44Z

I created a new PR to add apns-push-type support to the new master in our fork: parse-community/node-apn#2

I can update the package dependency in this PR when the changes got merged there and a new version has been released.

davimacedo · 2019-07-26T08:05:18Z

Thanks again. I've just merged and released a new version: https://github.com/parse-community/node-apn/releases/tag/v3.0.1-parse

funkenstrahlen · 2019-07-26T08:14:56Z

Package dependency has been updated to the new release.

davimacedo · 2019-07-26T08:45:53Z

Thank you so much @funkenstrahlen ! I've just merged!

funkenstrahlen · 2019-07-26T09:20:20Z

Awesome! So it's time for a new release?

Thanks for your work too! I enjoy working together with you :)

davimacedo · 2019-07-26T17:56:54Z

Sure! I've just opened the PR: https://github.com/parse-community/parse-server-push-adapter/pull/135/files

parse-github-assistant · 2021-12-06T13:59:30Z

The label type:feature cannot be used here.

add support for apns-push-type

1a583b3

Required when delivering notifications to devices running iOS 13 and later, or watchOS 6 and later.

funkenstrahlen changed the title ~~add support for apns-push-type~~ Important: Add support for apns-push-type Jun 18, 2019

funkenstrahlen added enhancement labels Jun 24, 2019

funkenstrahlen added 5 commits July 13, 2019 11:12

Merge branch 'master' into apns-push-type

efadad8

set default value for push type to alert

557bfd2

If defined explicitly the passed value for push type is used.

adjust test case to also cover push type background

be1a997

refactor: fix indentation

b743417

refactor: add missing semicolons

0c07cb7

funkenstrahlen mentioned this pull request Jul 13, 2019

document push type and priority keys for notifications parse-community/docs#639

Merged

davimacedo previously approved these changes Jul 14, 2019

View reviewed changes

funkenstrahlen changed the title ~~Important: Add support for apns-push-type~~ Add support for apns-push-type Jul 24, 2019

funkenstrahlen added 2 commits July 25, 2019 09:43

use parse community fork of apn

fa92562

run npm update to pull latest packages

6d35840

funkenstrahlen dismissed davimacedo’s stale review via 6d35840 July 25, 2019 07:46

update node-apn dependency to 3.0.1 release

7a60de8

Merge branch 'master' into apns-push-type

6421e73

davimacedo approved these changes Jul 26, 2019

View reviewed changes

davimacedo merged commit 175c261 into parse-community:master Jul 26, 2019

funkenstrahlen removed the GH Review: needs-revision label Jul 26, 2019

mtrezza added type:feature and removed type:improvement labels Dec 6, 2021

parse-github-assistant bot removed the type:feature label Dec 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for apns-push-type #127

Add support for apns-push-type #127

funkenstrahlen commented Jun 9, 2019 •

edited

Loading

codecov bot commented Jun 9, 2019 •

edited

Loading

funkenstrahlen commented Jun 9, 2019

funkenstrahlen commented Jun 20, 2019

dplewis commented Jun 20, 2019

funkenstrahlen commented Jun 21, 2019 •

edited

Loading

funkenstrahlen commented Jun 21, 2019

funkenstrahlen commented Jun 27, 2019 •

edited

Loading

dplewis commented Jul 11, 2019

acinader commented Jul 11, 2019

davimacedo commented Jul 11, 2019

funkenstrahlen commented Jul 12, 2019

davimacedo commented Jul 12, 2019

funkenstrahlen commented Jul 12, 2019

funkenstrahlen commented Jul 13, 2019

funkenstrahlen commented Jul 13, 2019

davimacedo commented Jul 14, 2019

funkenstrahlen commented Jul 14, 2019 •

edited

Loading

mman commented Jul 14, 2019

davimacedo commented Jul 14, 2019

funkenstrahlen commented Jul 14, 2019

davimacedo commented Jul 25, 2019

funkenstrahlen commented Jul 25, 2019 •

edited

Loading

funkenstrahlen commented Jul 25, 2019

davimacedo commented Jul 25, 2019

funkenstrahlen commented Jul 25, 2019 •

edited

Loading

davimacedo commented Jul 25, 2019

funkenstrahlen commented Jul 26, 2019 •

edited

Loading

davimacedo commented Jul 26, 2019

funkenstrahlen commented Jul 26, 2019

davimacedo commented Jul 26, 2019

funkenstrahlen commented Jul 26, 2019

davimacedo commented Jul 26, 2019

parse-github-assistant bot commented Dec 6, 2021

Add support for apns-push-type #127

Add support for apns-push-type #127

Conversation

funkenstrahlen commented Jun 9, 2019 • edited Loading

codecov bot commented Jun 9, 2019 • edited Loading

Codecov Report

funkenstrahlen commented Jun 9, 2019

funkenstrahlen commented Jun 20, 2019

dplewis commented Jun 20, 2019

funkenstrahlen commented Jun 21, 2019 • edited Loading

funkenstrahlen commented Jun 21, 2019

funkenstrahlen commented Jun 27, 2019 • edited Loading

dplewis commented Jul 11, 2019

acinader commented Jul 11, 2019

davimacedo commented Jul 11, 2019

funkenstrahlen commented Jul 12, 2019

davimacedo commented Jul 12, 2019

funkenstrahlen commented Jul 12, 2019

funkenstrahlen commented Jul 13, 2019

funkenstrahlen commented Jul 13, 2019

davimacedo commented Jul 14, 2019

funkenstrahlen commented Jul 14, 2019 • edited Loading

mman commented Jul 14, 2019

davimacedo commented Jul 14, 2019

funkenstrahlen commented Jul 14, 2019

davimacedo commented Jul 25, 2019

funkenstrahlen commented Jul 25, 2019 • edited Loading

funkenstrahlen commented Jul 25, 2019

davimacedo commented Jul 25, 2019

funkenstrahlen commented Jul 25, 2019 • edited Loading

davimacedo commented Jul 25, 2019

funkenstrahlen commented Jul 26, 2019 • edited Loading

davimacedo commented Jul 26, 2019

funkenstrahlen commented Jul 26, 2019

davimacedo commented Jul 26, 2019

funkenstrahlen commented Jul 26, 2019

davimacedo commented Jul 26, 2019

parse-github-assistant bot commented Dec 6, 2021

funkenstrahlen commented Jun 9, 2019 •

edited

Loading

codecov bot commented Jun 9, 2019 •

edited

Loading

funkenstrahlen commented Jun 21, 2019 •

edited

Loading

funkenstrahlen commented Jun 27, 2019 •

edited

Loading

funkenstrahlen commented Jul 14, 2019 •

edited

Loading

funkenstrahlen commented Jul 25, 2019 •

edited

Loading

funkenstrahlen commented Jul 25, 2019 •

edited

Loading

funkenstrahlen commented Jul 26, 2019 •

edited

Loading