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

[keep-awake] Add a flag to enable or disable useKeepAwake. #28908

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

macksal
Copy link
Contributor

@macksal macksal commented May 16, 2024

Why

useKeepAwake is a nice declarative interface for activating KeepAwake for as long as a component is on-screen. Great!

Now, let's only keep the screen awake when in a particular state, e.g. while a video or animation is playing.

const MyVideoPlayer = () => {
    const [isPlaying, setPlaying] = useState(true);
    // ❌ oops, we can't do this
    if (isPlaying) { useKeepAwake(); }
    // ...
}

We can always wrap the hook in a component to get around this, but it's not very ergonomic:

const KeepAwake = () => { useKeepAwake(); return null; }

const MyVideoPlayer = () => {
    const [isPlaying, setPlaying] = useState(true);
    // ...
    return <>
        {isPlaying && <KeepAwake />}
        <MyOtherComponents />
    </>
}

What if we could do something like this instead?

const MyVideoPlayer = () => {
    const [isPlaying, setPlaying] = useState(true);
    useKeepAwake({ enabled: isPlaying });
    // ...
}

How

Let's add a boolean option enabled to useKeepAwake that allows for the effect to be disabled. If the caller doesn't provide this option, the the effect is always active (the same as the current behaviour).

When the enabled option is exactly false, we simply don't do anything in the useEffect.

Side note

The resulting interface is still a little bit awkward since the user still needs to pass the tag parameter, which should most often be undefined:

useKeepAwake(undefined, { enabled: true });

It'd be nice to ad an overload allowing for the options object to be the first parameter. I doubt that providing tag is more frequent than providing options especially after #28884. Personally, I'd move tag inside the options parameter and deprecate the current call signature, but let's think about that later.

Test Plan

  • Added unit tests to check KeepAwake is activated when enabled is true or undefined/absent, and not activated when enabled is false.
  • Added a unit test to verify KeepAwake is toggled when enabled changes for subsequent re-renders.
  • Confirmed the existing functionality is unchanged, as verified by the existing tests.

Checklist

@macksal macksal requested a review from brentvatne as a code owner May 16, 2024 03:27
@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label May 16, 2024
@macksal macksal force-pushed the feature/useKeepAwake-enabled-flag branch from 910bc78 to 5a3e3ab Compare May 16, 2024 03:30
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels May 16, 2024
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.

None yet

2 participants