funkenstrahlen commented 5 years ago

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: https://github.com/node-apn/node-apn/pull/656

codecov[bot] commented 5 years ago

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%> (ø)`	:arrow_up:

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

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

funkenstrahlen commented 5 years ago

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

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

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

funkenstrahlen commented 5 years ago

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

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

@acinader Thoughts?

acinader commented 4 years ago

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

"(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 commented 4 years ago

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

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

funkenstrahlen commented 4 years ago

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

funkenstrahlen commented 4 years ago

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

I also created a PR for documenting this change: https://github.com/parse-community/docs/pull/639

davimacedo commented 4 years ago

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

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

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

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 https://github.com/parse-community/node-apn/pull/1 ? @funkenstrahlen could you also change our dependency to pin our fork in this PR?

funkenstrahlen commented 4 years ago

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

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

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

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

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

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

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

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

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

davimacedo commented 4 years ago

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

Package dependency has been updated to the new release.

davimacedo commented 4 years ago

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

funkenstrahlen commented 4 years ago

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

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

davimacedo commented 4 years ago

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

parse-github-assistant[bot] commented 2 years ago

The label type:feature cannot be used here.

parse-community / parse-server-push-adapter

Add support for apns-push-type #127

Codecov Report