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

[ Small Optimization ] Excess useEffect dependency #15

Open
gclark-eightfold opened this issue Oct 21, 2021 · 2 comments
Open

[ Small Optimization ] Excess useEffect dependency #15

gclark-eightfold opened this issue Oct 21, 2021 · 2 comments

Comments

@gclark-eightfold
Copy link

gclark-eightfold commented Oct 21, 2021

Note: I could make a PR for this if this package is still being maintained and is open to PRs, but seeing as the directory structure only includes a dist folder, I assume this package only includes the JS-compiled package.

I was looking to wrap the RN AppState in my own custom hook and came across this package. While there's some functionality I still wish to add myself, I noticed that it looked like the hook has appState as a dependency, which creates a lot of excess listener creations and cleanups.

By modifying the handleAppStateChange function to:

function handleAppStateChange(nextAppState) {
  setAppState((prevAppState) => {
    if (nextAppState === 'active' && prevAppState !== 'active')
      isValidFunction(onForeground) && onForeground();
    else if (prevAppState === 'active' && nextAppState.match(/inactive|background/))
      isValidFunction(onBackground) && onBackground();
  });
  isValidFunction(onChange) && onChange(nextAppState);
}

and make the dependency list: [onChange, onForeground, onBackground] so that changes to the appState do not trigger the creation of a new AppState listener after the previous listener is cleaned up.

This change isn't strictly necessary, since the hook's appState and callback function calls remain unaffected by the number of times the change listeners are added/removed, but it was just something I noticed while testing the two methods.

@laffed
Copy link

laffed commented Mar 3, 2022

@gclark-eightfold forgot to return new appState in setAppState

@alielmajdaoui
Copy link

alielmajdaoui commented Jan 26, 2023

@laffed it should be like this:

    useEffect(() => {
        function handleAppStateChange(nextAppState) {
            setAppState((prevAppState) => {
                if (nextAppState === 'active' && prevAppState !== 'active') {
                    isValidFunction(onForeground) && onForeground();
                } else if (prevAppState === 'active' && nextAppState.match(/inactive|background/)) {
                    isValidFunction(onBackground) && onBackground();
                }

                return nextAppState;
            });
            isValidFunction(onChange) && onChange(nextAppState);
        }
    }, [onChange, onForeground, onBackground]);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants