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

[expo][keep-awake] Fix Unable to deactivate keep awake warnings #17319

Merged
merged 5 commits into from
May 5, 2022

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented May 3, 2022

Why

a slightly different proposal of #13432 and to fix #12584

my hypothesis is that in the case, the Activity is dead (finish() called) but the process is still alive and the react-native bridge is still alive. before the Activity dead, deactivateKeepAwake is called. however, the unhandled promise rejection will be kept in the bridge queue. it will show in the yellow box when next time the new Activity created.

sometimes, android will not finish Activity when pressing back button, especially on android 12. to reliably test the issue, i would turn on the Don't keep activities flag in android developer menu:

according to the study from #13432, the FLAG_KEEP_SCREEN_ON scope is window based. if the Activity is dead, we don't need to remove the flag.

How

based on the observations above, my assumption is that it's a common case and we don't need to show the warning. this pr is to remove the warning.

  • update activateKeepAwake and deactivateKeepAwake to return Promise<void>, so that we can handle to promise rejection.
  • add { suppressDeactivateWarnings: true } option to suppress the promise rejection.
  • use the { suppressDeactivateWarnings: true } in withExpoRoot

Test Plan

Integration Test

  1. turn on Don't keep activities flag in android developer menu.
  2. on a sdk45 project
  3. launch app -> press back (triangle) button -> press square button to resume the app. see whether the warning is displayed.

Unit Test

  useKeepAwake
    ✓ test default calls without any parameters (6 ms)
    ✓ test explicit calls with parameters (1 ms)

i did try to test the promise rejection. i got no luck to test the promise rejection in hook as well as the useEffect cleanup function.

Checklist

  • Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md).
  • This diff will work correctly for expo build (eg: updated @expo/xdl).
  • This diff will work correctly for expo prebuild & EAS Build (eg: updated a module plugin).

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label May 3, 2022
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels May 3, 2022
@brentvatne brentvatne requested a review from byCedric May 3, 2022 17:52
@Kudo Kudo marked this pull request as ready for review May 3, 2022 18:12
Copy link
Member

@byCedric byCedric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gentleman-club

@Kudo
Copy link
Contributor Author

Kudo commented May 4, 2022

noticed the ios test suite ci job broken from my accidental copied change. added the last change to fix it 🤞

@Kudo
Copy link
Contributor Author

Kudo commented May 5, 2022

the ios ci jobs failure may come from pod cache with this commit on main branch. i'll merge this pr anyway and open another pr to fix the ios failure.

@Kudo Kudo merged commit 3bbd660 into main May 5, 2022
@Kudo Kudo deleted the @kudo/sdk45/fix-12584-2 branch May 5, 2022 02:17
Kudo added a commit that referenced this pull request May 5, 2022
…7319)

# Why

a slightly different proposal of #13432 and to fix #12584

my hypothesis is that in the case, the Activity is dead (`finish()` called) but the process is still alive and the react-native bridge is still alive. before the Activity dead, `deactivateKeepAwake` is called. however, the unhandled promise rejection will be kept in the bridge queue. it will show in the yellow box when next time the new Activity created.

sometimes, android will not finish Activity when pressing back button, [especially on android 12](https://developer.android.com/about/versions/12/behavior-changes-all#back-press). to reliably test the issue, i would turn on the `Don't keep activities` flag in android developer menu:
<img src="https://user-images.githubusercontent.com/46429/166503633-5d8cfe46-599a-4b35-8f1c-d228a5ab0a40.png" height="500" />

according to the study from #13432, the `FLAG_KEEP_SCREEN_ON` scope is window based. if the Activity is dead, we don't need to remove the flag.

# How

based on the observations above, my assumption is that it's a common case and we don't need to show the warning. this pr is to remove the warning.

- update `activateKeepAwake` and `deactivateKeepAwake` to return `Promise<void>`, so that we can handle to promise rejection.
- add `{ suppressDeactivateWarnings: true }` option to suppress the promise rejection.
- use the `{ suppressDeactivateWarnings: true }` in `withExpoRoot`

# Test Plan

#### Integration Test

1. turn on `Don't keep activities` flag in android developer menu.
2. on a sdk45 project
3. launch app -> press back (triangle) button -> press square button to resume the app. see whether the warning is displayed.

#### Unit Test

```
  useKeepAwake
    ✓ test default calls without any parameters (6 ms)
    ✓ test explicit calls with parameters (1 ms)
```

i did try to test the promise rejection. i got no luck to test the promise rejection in hook as well as the `useEffect` cleanup function.

(cherry picked from commit 3bbd660)
@siarheipashkevich
Copy link
Contributor

image

On the activation we also have the problem (

@Kudo
Copy link
Contributor Author

Kudo commented Apr 18, 2023

@siarheipashkevich would be happy to investigate if you could have a minimal reproducible example.

@siarheipashkevich
Copy link
Contributor

siarheipashkevich commented Apr 18, 2023

@Kudo it's very simple we should switch to another tab before the page is loaded. And when I back to tab with my app I saw this error, I think it's because the page is not loaded when the keep awake should be activated.. if you need I can record video

@Kudo
Copy link
Contributor Author

Kudo commented Apr 18, 2023

@siarheipashkevich having these steps and it works for me. so it would be good if you could create a reproducible project for us to investigate.

$ yarn create expo-app -t tabs@sdk-48 sdk48
$ cd sdk48
$ npx expo start -w

@siarheipashkevich
Copy link
Contributor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error: Unable to deactivate keep awake. However, it probably is deactivated already.
7 participants