Skip to content
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

notifs: Token-registration code hard to follow, and different per-platform #5329

Open
chrisbobbe opened this issue Apr 7, 2022 · 0 comments

Comments

@chrisbobbe
Copy link
Contributor

(Largely copied from #5300 (comment).)

I've often found our code for registering push tokens pretty hard to follow, and wondered if we could make it better.

We quick-fixed a P0 bug #4573 with 6ffc7e1, and I don't think we've settled on a plan for something more permanent.

Here's what I think the relevant subsystem is aimed at:

For all servers we want notifications from, we should keep the server up-to-date with our current device token.

  • If we're logged into a server, we want notifications from it.
    • This suggests having some per-account code that runs with the account's LOGIN_SUCCESS.
  • Our device token could change between sessions of running the app, or during a session.
    • This suggests having some global code that runs on startup, and on learning (e.g., from an event) that the token has changed during this session. Global because the token is for the device, and also because we may need to tell multiple servers about the token.

I think there's a main reason why our code has developed such that it's not super clear how we answer these requirements: we're not supposed to ask iOS for the token until we're ready to pop up a permissions request, and we're supposed to do that only in a context where the permission would be useful to have. We've (reasonably) decided that the app enters that context when a /register completes, so that's when we request the token on iOS.

Here's an example of how the code is hard to follow. It occurred to me after your comment prompted me to keep thinking about this:

  • Our device token could change between sessions of running the app […].
    • This suggests having some global code that runs on startup […]. Global because the token is for the device, and also because we may need to tell multiple servers about the token.

Our "on-startup" code for this is initNotifications. That sure doesn't look like "some global code", does it? It's a ThunkAction, not a GlobalThunkAction. But then, if you sit and think about it and read some distant code, it's possible to convince yourself that all interested servers will indeed be notified if the device token changed between sessions of running the app, on iOS and Android. Better if the "on-startup" code were instead a GlobalThunkAction, with a nice jsdoc that mentioned that device tokens can change between sessions.

I have a draft from a year ago for a unified getAndReturnNotificationToken function that grabs the token in the platform-appropriate way. That specific draft may or may not end up being helpful, but I think if we chose one of the following, we could unify our separate iOS/Android code and thus remove lots of complexity:

  1. iOS and Android both get the token immediately on startup (sad for iOS because the permission pop-up isn't deferred till after /register), or
  2. iOS and Android both get the token after /register, where iOS gets it currently (maybe sad for Android because Android doesn't have this requirement that iOS has—but also it wouldn't be harmful, would it?)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant