@mczernek/notifications suspended after 5 minutes #9287

mczernek · 2020-07-17T15:44:12Z

Why

Current architecture of expo-notifications is based in many places on invoking JobIntentService for each small action. While it provided nice and concise way of handling asynchronous tasks, it cause some of those actions to be deffered until Android decides to perform them. That meant, that after five minutes of inactivity on the phone (blocked and disconnected from charger), those actions were queued for later execution.

One of such actions was handling of just received notification. As a result, after receiving call from remote (FCM) notification was not displayed right away, even though app was being waked by system.

It is described in #9037

How

issue is fixed by converting most of those actions from execution in JobIntentService into straight synchronous code.

Mentioned service served not only as as asynchronity provider, but as a way of scoping for Notifications. Since some code in the module is executed without ModuleRegistry, we could not use standard scoping mechanism. As a result, I opted to creating bare and expoview flavors. Thanks to small modification in gradle.groovy this should come unnoticed to users and expo developers. At the same time it provides great way of scoping when we can't use ModuleRegistry.

Test Plan

test-suite and ncl - both bare and expo client.

…ebaseService.

…by FirebaseService, but is not JobIntentService.

…action.

Add bare and expoview flavors to expomodule in order to provide different implementations for those workflows.

…ule-template.

packages/expo-notifications/CHANGELOG.md

github-actions · 2020-07-17T16:21:58Z

	Fails
🚫	📋 Missing Changelog 🛠 Add missing entries to: `packages/react-native-unimodules/CHANGELOG.md` `packages/unimodules-app-loader/CHANGELOG.md`

🛠 Suggested fixes:

📋 Missing changelog

Apply suggested changes:

diff --git a/packages/react-native-unimodules/CHANGELOG.md b/packages/react-native-unimodules/CHANGELOG.md
index 56ffe442..591b8a71 100644
--- a/packages/react-native-unimodules/CHANGELOG.md
+++ b/packages/react-native-unimodules/CHANGELOG.md
@@ -10,6 +10,8 @@
 
 ### 🐛 Bug fixes
 
+- @mczernek/notifications suspended after 5 minutes. ([#9287](https://github.com/expo/expo/pull/9287) by [@mczernek](https://github.com/mczernek))
+
 ## 0.10.1 — 2020-05-29
 
 ### 📚 Library updates

diff --git a/packages/unimodules-app-loader/CHANGELOG.md b/packages/unimodules-app-loader/CHANGELOG.md
index 12960222..67b9c3a4 100644
--- a/packages/unimodules-app-loader/CHANGELOG.md
+++ b/packages/unimodules-app-loader/CHANGELOG.md
@@ -8,6 +8,8 @@
 
 ### 🐛 Bug fixes
 
+- @mczernek/notifications suspended after 5 minutes. ([#9287](https://github.com/expo/expo/pull/9287) by [@mczernek](https://github.com/mczernek))
+
 ## 1.2.0 — 2020-05-29
 
 *This version does not introduce any user-facing changes.*

Generated by 🚫 dangerJS against 0f274e7

github-actions · 2020-07-17T16:23:49Z

Native Component List for this branch is ready

esamelson

Thanks for jumping on this! My main comment is that it seems a little bit overkill to add the whole expoview flavor and infrastructure to this module just for the one Reconstructor class. Is it possible to just keep all the expoview code in the root android/src/expoview directory and override this class from there? I think that as much as possible, the packages should not need to know about the Expo client and when we do need to have special case code for the client, that code should live in the client's codebase rather than in the individual packages. The packages themselves should not need to worry about supporting the Expo client special case and I think it's more confusing to have that code be distributed throughout all the packages rather than collated in one location.

Otherwise it lgtm, but would also want to make sure @lukmccall OKs the architecture changes in this PR before landing, since @sjchmiela is currently OOO.

mczernek · 2020-07-21T12:53:51Z

Thanks for jumping on this! My main comment is that it seems a little bit overkill to add the whole expoview flavor and infrastructure to this module just for the one Reconstructor class. Is it possible to just keep all the expoview code in the root android/src/expoview directory and override this class from there? I think that as much as possible, the packages should not need to know about the Expo client and when we do need to have special case code for the client, that code should live in the client's codebase rather than in the individual packages. The packages themselves should not need to worry about supporting the Expo client special case and I think it's more confusing to have that code be distributed throughout all the packages rather than collated in one location.

Otherwise it lgtm, but would also want to make sure @lukmccall OKs the architecture changes in this PR before landing, since @sjchmiela is currently OOO.

I do not know of any reliable way of substituting class implementation other than android flavors. We do have mechanism for that based on ModuleRegistry, but it is not applicable, since we do not have access to it.

Another solution I implemented in expo-task-manager is by declaring actual implementation in AndroidManifest's meta-data. I find flavor solutions much friendlier and was considering switching to it in task manager as well, given we got agreement on that.

Another solution I could potentially think of is avoiding scoping reconstructor whatsoever. I guess it would be doable. But instead of walking around the problem I've wanted to try and discuss a solution that seemed clean to me.

esamelson · 2020-07-22T01:41:56Z

Gotcha, thanks for the explanation. I would strongly lean towards the AndroidManifest solution that you used for expo-task-manager, for these reasons:

it puts the responsibility on expoview for enabling the extra scoping in the managed workflow (which should be treated as the special case here), rather than putting responsibility on react-native-unimodules to hide the extra flavor in bare workflow projects (which are the general case)
maintainability for developers touching the expoview code in the future. Confusion about how the class is being overridden in AndroidManifest can be headed off by explaining it in comments, but my opinion is that expoview as a library becomes harder to maintain if it's distributed around multiple directories/packages in this repo.

Happy to discuss/consider this further though!

lukmccall

Look good :D

I've left some comments ;)

lukmccall · 2020-07-22T07:52:15Z

...w/src/main/java/versioned/host/exp/exponent/modules/universal/ExpoModuleRegistryAdapter.java

@@ -78,6 +78,7 @@ public ExpoModuleRegistryAdapter(ReactModuleRegistryProvider moduleRegistryProvi
    moduleRegistry.registerExportedModule(new ScopedExpoNotificationPresentationModule(scopedContext, experienceId));
    moduleRegistry.registerInternalModule(new ScopedNotificationsChannelsProvider(scopedContext, experienceId));

+


Suggested change

🧹

lukmccall · 2020-07-22T08:27:01Z

apps/bare-expo/android/app/src/main/java/dev/expo/payments/generated/BasePackageList.java

@@ -59,6 +59,7 @@
        new expo.modules.speech.SpeechPackage(),
        new expo.modules.sqlite.SQLitePackage(),
        new expo.modules.taskManager.TaskManagerPackage(),
+        new expo.modules.updates.UpdatesPackage(),


I'm not sure if this line should be in this PR ;)

lukmccall · 2020-07-22T08:28:53Z

packages/expo-notifications/CHANGELOG.md

@@ -10,6 +10,7 @@

 ### 🐛 Bug fixes

+- Fix notifications not being displayed after five minutes of phone inactivity on Android ([#9287](https://github.com/expo/expo/pull/9287) by [@mczernek](https://github.com/mczernek))


Suggested change

- Fix notifications not being displayed after five minutes of phone inactivity on Android ([#9287](https://github.com/expo/expo/pull/9287) by [@mczernek](https://github.com/mczernek))

- Fix notifications not being displayed after five minutes of phone inactivity on Android. ([#9287](https://github.com/expo/expo/pull/9287) by [@mczernek](https://github.com/mczernek))

🧹

lukmccall · 2020-07-22T08:33:49Z

.../java/expo/modules/notifications/notifications/service/ExpoNotificationSchedulerService.java

@@ -344,7 +351,7 @@ protected void removeNotification(Context context, String identifier) {
  /**
   * Remove selected notifications from storage and cancel any pending alarms.
   *
-   * @param context Context this is being called from
+   * @param context     Context this is being called from


Suggested change

* @param context Context this is being called from

* @param context Context this is being called from

🧹

...ain/java/expo/modules/notifications/notifications/interfaces/NotificationsReconstructor.java

mczernek · 2020-07-22T09:44:51Z

Gotcha, thanks for the explanation. I would strongly lean towards the AndroidManifest solution that you used for expo-task-manager, for these reasons:

it puts the responsibility on expoview for enabling the extra scoping in the managed workflow (which should be treated as the special case here), rather than putting responsibility on react-native-unimodules to hide the extra flavor in bare workflow projects (which are the general case)

maintainability for developers touching the expoview code in the future. Confusion about how the class is being overridden in AndroidManifest can be headed off by explaining it in comments, but my opinion is that expoview as a library becomes harder to maintain if it's distributed around multiple directories/packages in this repo.

Happy to discuss/consider this further though!

I see your point and I could go with that. Will just have another try with convincing you to my solution :)

Flavours are very widely used concept in Android world, so it should not come with any surprise to those coming from Android. I believe we can get used to it as well :)
The only responsibility of react-native-unimodules is to set strategy for when flavour dimension is missing. In my humble opinion, that's much clearer than having class name typed in AndroidManifest's meta-data and recreating it with reflection.

Moreover, I am hoping that one day all scoping might be done with flavours. That would give us uniform and clear scoping with no need for reflection for that. Moreover, I can imagine in further future, that we just switch from client to standalone app by selecting the right flavour in expo repository.

Last but not least, I am all for having all module related code within the module. I believe it is much easier, while developing, say, expo-notifications to stick with expo-notifications module, rather than switching from respective module to expoview back and forth. I can see that is a matter of preference, though.

The way I see it, none of the solutions has clear advantage over the other. While I prefer mine, I can totally agree on rewriting it to yours given I failed to convince you :)

esamelson · 2020-07-23T02:32:06Z

Your points make sense as well 😄 but I still want to push back a little:

Flavours are very widely used concept in Android world, so it should not come with any surprise to those coming from Android. I believe we can get used to it as well :)
The only responsibility of react-native-unimodules is to set strategy for when flavour dimension is missing. In my humble opinion, that's much clearer than having class name typed in AndroidManifest's meta-data and recreating it with reflection.

My hesitation is not to the flavors themselves but to the fact that we would be shipping internal-only flavors in an external-facing package. We've seen that when complications due to our client codebase sneak into externally-consumed packages, this tends to make things weirder and more difficult for developers (ExpoKit is an extreme example, but even the unimodule architecture itself is something we want to reduce), so I would strongly prefer to keep the packages as lean and close to other plain RN libraries as possible. However, if this type of practice (shipping flavors used for internal builds) is common among Android libraries I could be convinced otherwise! 😄

Last but not least, I am all for having all module related code within the module. I believe it is much easier, while developing, say, expo-notifications to stick with expo-notifications module, rather than switching from respective module to expoview back and forth. I can see that is a matter of preference, though.

This makes a lot of sense. One point in favor of keeping the scoped expoview code together, though, is that this is the convention we already follow for all the other scoped modules (which use ModuleRegistry), so this one would be a bit of an outlier.

mczernek · 2020-07-23T08:19:16Z

As of your concern with shipping flavour specific code to users, we try to mitigate that by not publishing expoview specific code to npm in modules (as seen in all .npmignore files). There is still a risk that users might face problems here, especially when they try to set up unimodules on their own (without unimodules gradle plugin).

To sum up this discussion, we do not have one approach that would solve all the issues, but you still prefer yours to mine :) I am comfortable to migrate to yours then, just let me know my assumption is correct.

esamelson · 2020-07-24T00:21:01Z

OK, yeah, that sounds good. Thanks for your willingness to discuss/change this!

…expo-module-template." This reverts commit 306ed37.

…minutes

cruzach

I'm not as familiar as eric and lukasz with the rest of the expoview and expo-notifications infrastructure, respectively, so I defer to them for that, but this doesn't look like it breaks any of the recent notification category functionality, thanks 👍

...ost/exp/exponent/modules/universal/notifications/ScopedExpoNotificationCategoriesModule.java

lukmccall

LGTM 🚀🚀🚀

android/app/build.gradle

lukmccall · 2020-07-29T15:20:57Z

android/expoview/src/main/AndroidManifest.xml

-        <provider
-            android:name="versioned.host.exp.exponent.modules.api.components.webview.RNCWebViewFileProvider"
-            android:authorities="${applicationId}.fileprovider"
-            android:exported="false"
-            android:grantUriPermissions="true">
-            <meta-data
-                android:name="android.support.FILE_PROVIDER_PATHS"
-                android:resource="@xml/provider_paths" />
-        </provider>
+    <provider
+      android:name="androidx.core.content.FileProvider"
+      android:authorities="${applicationId}.provider"
+      android:exported="false"
+      android:grantUriPermissions="true">
+      <meta-data
+        android:name="android.support.FILE_PROVIDER_PATHS"
+        android:resource="@xml/provider_paths" />
+    </provider>


I'm not sure if this change should be in this PR ;)

Wow, that's weird. Must be some side effect of rebasing/upmerging. Will take a look at that.

Really, there is only change in formatting. But GH shows the diff quite weirdly.

packages/expo-notifications/android/build.gradle

...expo-notifications/android/src/main/java/expo/modules/notifications/NotificationsScoper.java

...in/java/expo/modules/notifications/notifications/interfaces/NotificationsBuilderCreator.java

...ain/java/expo/modules/notifications/notifications/interfaces/NotificationsReconstructor.java

lukmccall · 2020-07-29T15:47:22Z

.../java/expo/modules/notifications/notifications/service/ExpoNotificationSchedulerService.java

@@ -344,7 +351,7 @@ protected void removeNotification(Context context, String identifier) {
  /**
   * Remove selected notifications from storage and cancel any pending alarms.
   *
-   * @param context Context this is being called from
+   * @param context     Context this is being called from


packages/react-native-unimodules/gradle.groovy

…tes too (#9816) # Why Expansion of #9287. Before this pull request `ScheduledAlarmReceiver` enqueues work to `ExpoNotificationSchedulerService` which may be postponed by Android system indefinitely. Simply making the `ExpoNotificationSchedulerService` a POJO will make `ScheduledAlarmReceiver` not try to enqueue work to another service, but execute it immediately. # How Renamed `ExpoNotificationSchedulerService` to `NotificationSchedulingHelper` and made it a POJO with only the `enqueue*` methods being public and available. # Test Plan I've verified that tests related to scheduling pass in `test-suite`.

… flow (#10680) # Why Preparation to make `expo-notifications-categories-delegate` PR easier to review. See 42ffff4. # How Reverted changes made in #9287 so that categories managing uses `ResultReceiver`. # Test Plan I've verified that all relevant tests pass with `test-suite`.

mczernek added 9 commits July 16, 2020 16:27

Extract NotificationHelper, move notifications rendering logic to Fir…

f2a2dbd

…ebaseService.

Move creating notification to Expo specific service which is started …

e7d7f5a

…by FirebaseService, but is not JobIntentService.

Do not run seperate IntentService for every notifications associated …

3d172c5

…action.

[WIP]

565ee16

Scoping.

3428df7

Add flavors to expo module

f5c08d0

Add bare and expoview flavors to expomodule in order to provide different implementations for those workflows.

Prepare modules to be published with flavors. Add flavors to expo-mod…

306ed37

…ule-template.

Remove unnecessary expo-notifications-interface module.

0131338

CHANGELOG.

a591786

mczernek requested review from esamelson, sjchmiela, tsapeta and lukmccall July 17, 2020 15:48

mczernek marked this pull request as ready for review July 17, 2020 15:48

mczernek requested review from bbarthec, brentvatne, cruzach, EvanBacon, ide and IjzerenHein as code owners July 17, 2020 15:48

barthap reviewed Jul 17, 2020

View reviewed changes

packages/expo-notifications/CHANGELOG.md Outdated Show resolved Hide resolved

I am notbrent :)

6c398bf

esamelson reviewed Jul 20, 2020

View reviewed changes

lukmccall reviewed Jul 22, 2020

View reviewed changes

mczernek added 2 commits July 27, 2020 11:21

Revert "Prepare modules to be published with flavors. Add flavors to …

f5e36c4

…expo-module-template." This reverts commit 306ed37.

Remove flavours, scope via AndroidManifest.xml

5094bd1

mczernek force-pushed the @mczernek/notifications-suspended-after-5-minutes branch from f1a49ab to 5094bd1 Compare July 27, 2020 12:32

mczernek added 3 commits July 29, 2020 14:37

Merge branch 'master' into @mczernek/notifications-suspended-after-5-…

a8bfc29

…minutes

Correct scoping for notifications categories broken by upmerge.

8237358

Cleanup.

97f7afe

cruzach reviewed Jul 29, 2020

View reviewed changes

...ost/exp/exponent/modules/universal/notifications/ScopedExpoNotificationCategoriesModule.java Outdated Show resolved Hide resolved

lukmccall approved these changes Jul 29, 2020

View reviewed changes

mczernek added 2 commits July 30, 2020 09:31

Cleanup.

a0ea0a6

Reject for empty categories list.

0f274e7

mczernek merged commit d40fb97 into master Jul 30, 2020

mczernek deleted the @mczernek/notifications-suspended-after-5-minutes branch July 30, 2020 08:55

mczernek mentioned this pull request Jul 30, 2020

[SDK38] Android notifications stop after 5 minutes #9037

Closed

sjchmiela mentioned this pull request Aug 18, 2020

[expo-notifications] Ensure scheduled notifications work after 5 minutes too #9816

Merged

This was referenced Aug 19, 2020

Expo Push Notifications not received when app is closed [SDK 38.0.9] #9748

Closed

Expo-notifications, App closed notifications are not showing. #9163

Closed

cruzach mentioned this pull request Oct 8, 2020

[notifications][android] prevent crashing in standalone apps when receiving interactive notifications #10608

Merged

sjchmiela mentioned this pull request Oct 14, 2020

[expo-notifications] Migrate notifications categories to asynchronous flow #10680

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

@mczernek/notifications suspended after 5 minutes #9287

@mczernek/notifications suspended after 5 minutes #9287

mczernek commented Jul 17, 2020 •

edited

github-actions bot commented Jul 17, 2020 •

edited

📋 Missing Changelog

Apply suggested changes:

github-actions bot commented Jul 17, 2020

esamelson left a comment •

edited

mczernek commented Jul 21, 2020

esamelson commented Jul 22, 2020

lukmccall left a comment •

edited

lukmccall Jul 22, 2020

lukmccall Jul 22, 2020

lukmccall Jul 22, 2020

lukmccall Jul 22, 2020

lukmccall Jul 29, 2020

mczernek commented Jul 22, 2020

esamelson commented Jul 23, 2020

mczernek commented Jul 23, 2020

esamelson commented Jul 24, 2020

cruzach left a comment •

edited

lukmccall left a comment

lukmccall Jul 29, 2020

mczernek Jul 30, 2020

mczernek Jul 30, 2020

lukmccall Jul 29, 2020

		@@ -78,6 +78,7 @@ public ExpoModuleRegistryAdapter(ReactModuleRegistryProvider moduleRegistryProvi
		moduleRegistry.registerExportedModule(new ScopedExpoNotificationPresentationModule(scopedContext, experienceId));
		moduleRegistry.registerInternalModule(new ScopedNotificationsChannelsProvider(scopedContext, experienceId));

		@@ -10,6 +10,7 @@

		### 🐛 Bug fixes

		- Fix notifications not being displayed after five minutes of phone inactivity on Android ([#9287](https://github.com/expo/expo/pull/9287) by [@mczernek](https://github.com/mczernek))

	* @param context Context this is being called from
	* @param context Context this is being called from

@mczernek/notifications suspended after 5 minutes #9287

@mczernek/notifications suspended after 5 minutes #9287

Conversation

mczernek commented Jul 17, 2020 • edited

Why

How

Test Plan

github-actions bot commented Jul 17, 2020 • edited

📋 Missing Changelog

🛠 Suggested fixes:

Apply suggested changes:

github-actions bot commented Jul 17, 2020

esamelson left a comment • edited

Choose a reason for hiding this comment

mczernek commented Jul 21, 2020

esamelson commented Jul 22, 2020

lukmccall left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mczernek commented Jul 22, 2020

esamelson commented Jul 23, 2020

mczernek commented Jul 23, 2020

esamelson commented Jul 24, 2020

cruzach left a comment • edited

Choose a reason for hiding this comment

lukmccall left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mczernek commented Jul 17, 2020 •

edited

github-actions bot commented Jul 17, 2020 •

edited

esamelson left a comment •

edited

lukmccall left a comment •

edited

cruzach left a comment •

edited