-
-
Notifications
You must be signed in to change notification settings - Fork 67
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add the ability to manage channels and send broadcasts #164
base: master
Are you sure you want to change the base?
Conversation
Thanks for opening this pull request! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #164 +/- ##
==========================================
+ Coverage 93.53% 95.79% +2.25%
==========================================
Files 23 23
Lines 588 832 +244
==========================================
+ Hits 550 797 +247
+ Misses 38 35 -3 ☔ View full report in Codecov by Sentry. |
@mtrezza and @dplewis, can you do an initial review of this PR when you have a chance? To enable broadcasting, a larger amount of code changes are needed than normal, in particular because client.js hasn't been updated in 3+ years, was designed for original style push notifications (can only I have slowly integrated the changes the client needs to support broadcasting, and hopefully, it will be future-proofed so changes can be added more easily with less code. The test cases have been updated with the async/await code, but all of the original tests still ensure their expected results. I still have test cases to add, but I don't expect many changes to To help guide you through the code changes (again, my apologies for the large PR), I recommend the following:
I am open to any feedback/suggestions... |
@mtrezza the code for this is finished and ready for a full review. |
…into useBroadcastPath
…into useBroadcastPath
@@ -384,13 +384,6 @@ module.exports = function (dependencies) { | |||
this.destroySession(session); | |||
}); | |||
|
|||
this.session.on('socketError', error => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed this as it's no longer available in the older version of the node tested (version 14 LTS). The last time I see it is in node version 8.7.0.
I'm sure whatever it use to catch is still caught by the rest of the error handling in this promise
Issue
Continuation of #163 which allowed the additional properties needed for broadcast notifications, but doesn't modify the Client API to
GET
,POST
,DELETE
to the respective Apple paths.Closes: #162
Closes: #146
Closes: #111
Closes: #116
Approach
Follow the documentation related to channel management and broadcasting. This enables the functionality described in Apple's WWDC 2024 videos: Broadcast Updates to Your Live Activities.
The newly added API methods to
node-apn
will have the following footprint (TypeScript):Non-Breaking Changes
All changes are non-breaking as they are made to the client API, which isn't exposed to developers, and new methods that previously weren't available are added. The typescript changes are improvements but are non-breaking as well.
Additionally, the PR does the following: