Merge pull request #38 from react-navigation/bug/fix-events

Bug/fix events

Author: slorber

src/Hooks.ts

@@ -1,4 +1,10 @@
-import { useState, useContext, useEffect } from 'react';
+import {
+  useState,
+  useContext,
+  useLayoutEffect,
+  useRef,
+  useCallback,
+} from 'react';
 
 import {
   NavigationContext,
@@ -11,7 +17,15 @@ import {
 } from 'react-navigation';
 
 export function useNavigation<S>(): NavigationScreenProp<S & NavigationRoute> {
-  return useContext(NavigationContext as any);
+  const navigation = useContext(NavigationContext) as any; // TODO typing?
+  if (!navigation) {
+    throw new Error(
+      "react-navigation hooks require a navigation context but it couldn't be found. " +
+        "Make sure you didn't forget to create and render the react-navigation app container. " +
+        'If you need to access an optional navigation object, you can useContext(NavigationContext), which may return'
+    );
+  }
+  return navigation;
 }
 
 export function useNavigationParam<T extends keyof NavigationParams>(
@@ -28,69 +42,104 @@ export function useNavigationKey() {
   return useNavigation().state.key;
 }
 
-export function useNavigationEvents(handleEvt: NavigationEventCallback) {
+// Useful to access the latest user-provided value
+const useGetter = <S>(value: S): (() => S) => {
+  const ref = useRef(value);
+  useLayoutEffect(() => {
+    ref.current = value;
+  });
+  return useCallback(() => ref.current, [ref]);
+};
+
+export function useNavigationEvents(callback: NavigationEventCallback) {
   const navigation = useNavigation();
-  useEffect(
-    () => {
-      const subsA = navigation.addListener(
-        'action' as any // TODO should we remove it? it's not in the published typedefs
-        , handleEvt);
-      const subsWF = navigation.addListener('willFocus', handleEvt);
-      const subsDF = navigation.addListener('didFocus', handleEvt);
-      const subsWB = navigation.addListener('willBlur', handleEvt);
-      const subsDB = navigation.addListener('didBlur', handleEvt);
-      return () => {
-        subsA.remove();
-        subsWF.remove();
-        subsDF.remove();
-        subsWB.remove();
-        subsDB.remove();
-      };
-    },
-    // For TODO consideration: If the events are tied to the navigation object and the key
-    // identifies the nav object, then we should probably pass [navigation.state.key] here, to
-    // make sure react doesn't needlessly detach and re-attach this effect. In practice this
-    // seems to cause troubles
-    undefined
-    // [navigation.state.key]
-  );
+
+  // Closure might change over time and capture some variables
+  // It's important to fire the latest closure provided by the user
+  const getLatestCallback = useGetter(callback);
+
+  // It's important to useLayoutEffect because we want to ensure we subscribe synchronously to the mounting
+  // of the component, similarly to what would happen if we did use componentDidMount
+  // (that we use in <NavigationEvents/>)
+  // When mounting/focusing a new screen and subscribing to focus, the focus event should be fired
+  // It wouldn't fire if we did subscribe with useEffect()
+  useLayoutEffect(() => {
+    const subscribedCallback: NavigationEventCallback = event => {
+      const latestCallback = getLatestCallback();
+      latestCallback(event);
+    };
+
+    const subs = [
+      // TODO should we remove "action" here? it's not in the published typedefs
+      navigation.addListener('action' as any, subscribedCallback),
+      navigation.addListener('willFocus', subscribedCallback),
+      navigation.addListener('didFocus', subscribedCallback),
+      navigation.addListener('willBlur', subscribedCallback),
+      navigation.addListener('didBlur', subscribedCallback),
+    ];
+    return () => {
+      subs.forEach(sub => sub.remove());
+    };
+  }, [navigation.state.key]);
+}
+
+export interface FocusState {
+  isFocused: boolean;
+  isBlurring: boolean;
+  isBlurred: boolean;
+  isFocusing: boolean;
 }
 
-const emptyFocusState = {
+const emptyFocusState: FocusState = {
   isFocused: false,
   isBlurring: false,
   isBlurred: false,
   isFocusing: false,
 };
-const didFocusState = { ...emptyFocusState, isFocused: true };
-const willBlurState = { ...emptyFocusState, isBlurring: true };
-const didBlurState = { ...emptyFocusState, isBlurred: true };
-const willFocusState = { ...emptyFocusState, isFocusing: true };
-const getInitialFocusState = (isFocused: boolean) =>
-  isFocused ? didFocusState : didBlurState;
-function focusStateOfEvent(eventName: EventType) {
+const didFocusState: FocusState = { ...emptyFocusState, isFocused: true };
+const willBlurState: FocusState = { ...emptyFocusState, isBlurring: true };
+const didBlurState: FocusState = { ...emptyFocusState, isBlurred: true };
+const willFocusState: FocusState = { ...emptyFocusState, isFocusing: true };
+
+function nextFocusState(
+  eventName: EventType,
+  currentState: FocusState
+): FocusState {
   switch (eventName) {
+    case 'willFocus':
+      return {
+        ...willFocusState,
+        // /!\ willFocus will fire on screen mount, while the screen is already marked as focused.
+        // In case of a new screen mounted/focused, we want to avoid a isFocused = true => false => true transition
+        // So we don't put the "false" here and ensure the attribute remains as before
+        // Currently I think the behavior of the event system on mount is not very well specified
+        // See also https://twitter.com/sebastienlorber/status/1166986080966578176
+        isFocused: currentState.isFocused,
+      };
     case 'didFocus':
       return didFocusState;
-    case 'willFocus':
-      return willFocusState;
     case 'willBlur':
       return willBlurState;
     case 'didBlur':
       return didBlurState;
     default:
-      return null;
+      // preserve current state for other events ("action"?)
+      return currentState;
   }
 }
 
 export function useFocusState() {
   const navigation = useNavigation();
-  const isFocused = navigation.isFocused();
-  const [focusState, setFocusState] = useState(getInitialFocusState(isFocused));
-  function handleEvt(e: NavigationEventPayload) {
-    const newState = focusStateOfEvent(e.type);
-    newState && setFocusState(newState);
-  }
-  useNavigationEvents(handleEvt);
+
+  const [focusState, setFocusState] = useState<FocusState>(() => {
+    return navigation.isFocused() ? didFocusState : didBlurState;
+  });
+
+  useNavigationEvents((e: NavigationEventPayload) => {
+    setFocusState(currentFocusState =>
+      nextFocusState(e.type, currentFocusState)
+    );
+  });
+
   return focusState;
 }

tslint.json

@@ -8,7 +8,7 @@
   "defaultSeverity": "error",
   "jsRules": {},
   "rules": {
-    "quotemark": [true, "single", "jsx-double"],
+    "quotemark": [false],
     "ordered-imports": false,
     "object-literal-sort-keys": false,
     "arrow-parens": [true, "ban-single-arg-parens"],