useNavigationEvents do not update listeners across re-renders

[slorber]

Hi,

I just want to warn that this implementation is problematic:

export function useNavigationEvents(handleEvt) {
  const navigation = useNavigation();
  useEffect(
    () => {
      const subsA = navigation.addListener('action', 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]
  );
}

The key is needed, otherwise the events will be removed/added on every update.
Unfortunately when you add a didFocus listener on a focused view, the listener will fire immediately after adding (async), which has lead to an infinite loop on this issue: react-navigation/react-navigation#5058

I suggest as a solution to:

• store react-navigation listeners in a ref to make them stable
• wire the user provided listeners to the stable listeners of the ref (so that user is still able to provide arrow functions/unstable listeners, and the latest provided version will be called, instead of the one provided on initial render)
• only update react-navigation listeners on some specific conditions, like key change?

I can try to provide an implementation if you want

[slorber]

Note your implementation using [key] was probably causing troubles because the listener that gets called is the initial arrow function, and does not update as component re-render. The closure might catch variables and change behavior other time.

function MyComp(({someBool})) {
  useNavigationEvents(evt => {
    if ( someBool ) {
      alert("true");
    }
    else {
      alert("false");
    }
  });
  ...
}

If somebool goes from true to false, the behavior of the closure (which has captured the boolean state) change other time. It will keep alerting the initial boolean value, regardless of its current value.

[ericvicenti]

Hey Sébastien, sorry for the delayed response!

I think it would make sense to only subscribe or re-subscribe when the key changes. I had attempted to do this by providing [navigation.state.key] as the second argument to useEffect, but this caused some pretty bad bugs and I haven't had time to investigate.

Can you help debug and fix this?

[slorber]

Yes sure I can try to make a PR soon. Do you have any idea how I could reproduce your "pretty bad bug" caused by providing navigation key? Just to make sure I clear it

[slorber]

I've tried running your dev web server. For unknown reasons I'm unable to yarn link my local react-navigation-hooks correctly. What's your dev setup for this project?

I'm able to edit my local package from npm which is not ideal.

The problem you met is:

  const [events, setEvents] = useState([]);
  useNavigationEvents(evt => {
    // latest state on evt.state
    // prev state on evt.lastState
    // triggering navigation action on evt.action
    setEvents([...events, evt]);
    // evt.type is [will/did][Focus/Blur]
  });

On first render, the event list is empty. The closure capture events = [], and only the first render closure is registered to the event system, you basically, you keep adding the last event to an empty list, because the empty list was captured in the initial closure, and the list does not grow

[slorber]

Hi @ericvicenti

I've made a PR which permit to use a stable callback and only update the subscriptions on key change, and yet avoid the problems mentionned above.
https://github.com/react-navigation/react-navigation-hooks/pull/8/files

Note that with previous implementation (with no useEffect inputs) or with my fix, I still have weird bugs where the navigation listener stops firing after a few events. If I switch from docA to docB it works (I get action events), but as soon as I click to the focused doc again, I get "willBlur/didBlur" events, and then, the listener provided to navigation.addListener never fires again.

Any idea what could be happening?

[slorber]

Also want to point out that the closure the user provide may fire multiple times synchronously, so you want to do this:

  const [events, setEvents] = useState([]);
  useNavigationEvents(evt => {
    setEvents(evts => evts.concat(evt));
  });

Instead of this:

  const [events, setEvents] = useState([]);
  useNavigationEvents(evt => {
    setEvents([...events,evt]);
  });

With the later, of you get willBlur/didBlur firing synchronously, you will get the same closure problem and only capture the last event, because setState is not synchronous

[slorber]

Will be fixed by #38, going to merge/release soon

[slorber]

should be fixed by release 1.0.3 and #38, please tell me if everything works