feat: Enable APNS registration + notification delivery in macOS apps #33574

joanx · 2022-04-01T19:14:01Z

Description of Change

Want to pick up on previous attempts #13777 and #18286 by @frantic and @kevinhu88 to enable APNS in Electron.

My understanding is that #18286 was blocked on concerns over supporting UserNotification and NSUserNotifications at the same time. This subset (APNS registration + notification receipt) doesn't actually seem to reference the UserNotification object (the relevant UserNotification -related code appears to have been handled in @miniak 's #25950.)

I've ported over the remaining functionality to register + receive APNS notifications into a separate fork (as the previous PR appears to be blocked on merge conflicts) and have confirmed successful registration + notification receipt via APNs with this sample app using Apple's command-line tools (note, I needed to create a provisioning profile + code-sign this app for successful APNS registration).

Would love to re-start the conversation around what it would take to enable APNS in Electron. Thanks in advance!

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
relevant documentation is changed or added
PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Added support for push notifications from APNs for macOS apps.

welcome · 2022-04-01T19:14:03Z

💖 Thanks for opening this pull request! 💖

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

fix: don't overwrite prevent_default if default wasn't prevented
feat: add app.isPackaged() method
docs: app.isDefaultProtocolClient is now available on Linux

Things that will help get your PR across the finish line:

Follow the JavaScript, C++, and Python coding style.
Run npm run lint locally to catch formatting errors earlier.
Document any user-facing changes you've made following the documentation styleguide.
Include tests when adding/changing behavior.
Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

joanx · 2022-04-01T19:20:09Z

shell/browser/browser_mac.mm

@@ -532,4 +532,34 @@ void RemoveFromLoginItems() {
  }
 }

+void Browser::RegisterForRemoteNotifications() {
+  [[AtomApplication sharedApplication]
+      registerForRemoteNotificationTypes:NSRemoteNotificationTypeBadge |


Attempting to use the updated API (registerForRemoteNotifications) resulted in this error:

'registerForRemoteNotifications' has been marked as being introduced in macOS 10.14 here, but the deployment target is macOS 10.11.0

zcbenz

API LGTM

joanx · 2022-04-05T17:34:14Z

@zcbenz thanks so much for the review! Is there any guidance on next steps to get this merged?

zcbenz · 2022-04-06T01:24:52Z

We just need to wait for a few more reviews from maintainers.

nornagon · 2022-04-11T22:50:10Z

cc @miniak and @MarshallOfSound who have worked in this area previously

michal-bednarz · 2022-04-28T12:04:11Z

Hey 👋 @nornagon @zcbenz @miniak @MarshallOfSound I just wanted to kindly ping you about this PR :) What are the next steps? Can we help somehow with pushing it into the release version?

MarshallOfSound · 2022-04-29T09:17:56Z

Haven't had a chance to look this over yet but I don't see CI runs this PR yet. @joanx can you try signing in to circleci and pushing a new commit to this PR. Builds should kick off after that

joanx · 2022-05-02T17:35:07Z

@MarshallOfSound just did!

MarshallOfSound · 2022-05-03T06:50:44Z

Few things:

Code looks fine in principle, the lint is unhappy and should be fixed
Idea is fine as well, I'm relatively OK with this API being added as it fundamentally has to be added into Electron if someone wants to use APNS
This is the part where I bikeshed over where to put the API:

I'm not a fan of lumping everything in with app.*, it's kinda a catch-all that is incredibly oversaturated at this point. I have two suggestions:

import { apns } from 'electron', specific to APNS
import { pushNotifications } from 'electron' if we think we may implement the windows equivalent
The existing Notification module, maybe Notification.push or Notification.apns

I'm open to arguments for either or an alternative, but I'm -0.5 on lumping this into app

joanx · 2022-05-09T22:48:25Z

@MarshallOfSound , thanks for the review. I'm unfortunately not familiar with the code organization and languages used here and unable to achieve a working binary with the refactor though I'd hugely appreciate any guidance on the in-progress changes.

Wondering if you have thoughts on any of these options:

Keep logic in app
Possible for me to pair with someone in the Electron maintainers group?
Possible for someone in the Electron maintainers group to own the refactor?
I continue to own these changes; there may be somewhat long iteration cycles as I ramp up on the necessary context to get this to a merge-ready state

Thank you!

tiagoporto · 2022-06-10T00:45:54Z

Hi @nornagon, could this PR move forward?

joanx · 2022-06-15T00:18:45Z

@tiagoporto there's still a bit of outstanding code review I'm hoping to get to shortly!

joanx · 2022-06-17T00:05:57Z

Hi @nornagon , thanks so much for the review. I believe I've addressed the comments, though am seeing some build failures that I'm not sure are related to my changes; would love any guidance. thanks!

edit: build is green now 🤔

nornagon

Overall looking pretty good! Now that I'm looking more closely at the API, I think the registerForAPNSNotifications function might be better expressed as a Promise, rather than as a function which returns void and triggers one of two events later on. It might be a little trickier to implement in the C++ side, though.

shell/browser/api/electron_api_push_notifications.cc

shell/browser/api/electron_api_push_notifications_mac.mm

shell/browser/mac/electron_application_delegate.mm

docs/api/push-notifications.md

joanx · 2022-06-29T21:36:38Z

Updated to promisify APNS registration method, and tested here

nornagon

awesome awesome. did you happen to check how macOS behaves if you call registerForAPNSNotifications() twice?

e.g. does this work with the new code?

const token1 = await pushNotifications.registerForAPNSNotifications()
const token2 = await pushNotifications.registerForAPNSNotifications()

does the 2nd promise properly resolve (or reject)? i.e. is it not left hanging forever? Also, is the token returned the same?

[edit] confirmed out-of-band that the above works as expected :)

shell/browser/api/electron_api_push_notifications_mac.mm

shell/browser/api/electron_api_push_notifications.h

joanx · 2022-06-29T22:08:01Z

Updated + re-tested!

shell/browser/api/electron_api_push_notifications_mac.mm

tiagoporto · 2022-06-30T14:37:44Z

Great job @joanx , looking forward to this merge.

welcome · 2022-07-12T15:38:52Z

Congrats on merging your first pull request! 🎉🎉🎉

release-clerk · 2022-07-12T15:38:53Z

Release Notes Persisted

Added support for push notifications from APNs for macOS apps.

KishanBagaria · 2022-08-02T05:06:29Z

Could this be backported to v20?

erickzhao · 2022-08-11T21:11:39Z

@KishanBagaria Unfortunately, we generally don't backport features to older branches at this point of the release cycle.

For more information on the process: https://github.com/electron/governance/blob/main/wg-releases/feature-backport-requests.md

joanx added 2 commits Mar 30, 2022

feat: enable APNS registration

d7b5d07

fix: add APNS register/unregister methods to app

bf05601

electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 1, 2022

joanx changed the title ~~Feat: Enable APNS registration + notification delivery in macOS apps~~ feat: Enable APNS registration + notification delivery in macOS apps Apr 1, 2022

joanx reviewed Apr 1, 2022

View changes

miniak mentioned this pull request Apr 2, 2022

feat: enable macOS apps to receive push notifications from APNs #18286

Closed

6 tasks

zcbenz added semver/minor backwards-compatible functionality api-review/requested 🗳 no-backport labels Apr 4, 2022

zcbenz approved these changes Apr 4, 2022

View changes

zcbenz self-assigned this Apr 13, 2022

minor wording edit (kick off CircleCI)

48f2573

joanx added 4 commits May 7, 2022

fix: lint

21b0770

fix: build errors

d619a96

fix: undo semantic override

4debddf

fix: clean up push_notifications header file

a2f0258

joanx added 5 commits May 18, 2022

fix: enable pushNotifications module visibility

b692619

fix: WIP resolve build errors

7e5f839

fix: add types, misc. cleanup

e842356

fix: type reference

9af65b5

fix: cleanup

1d60214

Merge branch 'main' into enable-apns

c3ea89e

joanx added 4 commits Jun 16, 2022

fix: decouple implementation from Browser object

5e78aae

fix: cleanup

7443a87

fix: add general push_notifications module

d590b5a

fix: cleanup

220e590

fix: rm unneeded imports

312ba2d

nornagon requested changes Jun 24, 2022

View changes

joanx added 4 commits Jun 24, 2022

fix: variable renaming + update get handler

055fafc

fix: cleanup

11055ce

fix: lint + update docs

8bc184a

fix: add example to docs

0508d7b

nornagon requested changes Jun 29, 2022

View changes

shell/browser/api/electron_api_push_notifications_mac.mm Outdated Show resolved Hide resolved

shell/browser/api/electron_api_push_notifications.h Outdated Show resolved Hide resolved

fix: naming + avoid list mutation

90443e5

nornagon reviewed Jun 29, 2022

View changes

shell/browser/api/electron_api_push_notifications_mac.mm Outdated Show resolved Hide resolved

fix: apns_promise_set as class variable

1d00714

nornagon approved these changes Jun 29, 2022

View changes

fix: lint

9802079

nornagon merged commit afd08c9 into electron:main Jul 12, 2022
13 checks passed

codebytere mentioned this pull request Jul 13, 2022

fix: base::DictionaryValue usage in APNS notifs #34897

Merged

commit-lint bot mentioned this pull request Sep 15, 2022

[pull] main from electron:main Blaquez-home/electron#3

Open

feat: Enable APNS registration + notification delivery in macOS apps #33574

feat: Enable APNS registration + notification delivery in macOS apps #33574

joanx commented Apr 1, 2022 •

edited

welcome bot commented Apr 1, 2022

joanx Apr 1, 2022

zcbenz left a comment

joanx commented Apr 5, 2022

zcbenz commented Apr 6, 2022

nornagon commented Apr 11, 2022

michal-bednarz commented Apr 28, 2022 •

edited

MarshallOfSound commented Apr 29, 2022

joanx commented May 2, 2022

MarshallOfSound commented May 3, 2022

joanx commented May 9, 2022

tiagoporto commented Jun 10, 2022

joanx commented Jun 15, 2022

joanx commented Jun 17, 2022 •

edited

nornagon left a comment

joanx commented Jun 29, 2022

nornagon left a comment •

edited

joanx commented Jun 29, 2022

tiagoporto commented Jun 30, 2022

welcome bot commented Jul 12, 2022

release-clerk bot commented Jul 12, 2022

KishanBagaria commented Aug 2, 2022

erickzhao commented Aug 11, 2022

feat: Enable APNS registration + notification delivery in macOS apps #33574

feat: Enable APNS registration + notification delivery in macOS apps #33574

Conversation

joanx commented Apr 1, 2022 • edited

Description of Change

Checklist

Release Notes

welcome bot commented Apr 1, 2022

joanx Apr 1, 2022

Choose a reason for hiding this comment

zcbenz left a comment

joanx commented Apr 5, 2022

zcbenz commented Apr 6, 2022

nornagon commented Apr 11, 2022

michal-bednarz commented Apr 28, 2022 • edited

MarshallOfSound commented Apr 29, 2022

joanx commented May 2, 2022

MarshallOfSound commented May 3, 2022

joanx commented May 9, 2022

tiagoporto commented Jun 10, 2022

joanx commented Jun 15, 2022

joanx commented Jun 17, 2022 • edited

nornagon left a comment

joanx commented Jun 29, 2022

nornagon left a comment • edited

joanx commented Jun 29, 2022

tiagoporto commented Jun 30, 2022

welcome bot commented Jul 12, 2022

release-clerk bot commented Jul 12, 2022

KishanBagaria commented Aug 2, 2022

erickzhao commented Aug 11, 2022

joanx commented Apr 1, 2022 •

edited

michal-bednarz commented Apr 28, 2022 •

edited

joanx commented Jun 17, 2022 •

edited

nornagon left a comment •

edited