Add stable actions on useNavigation

[neiker]

This solves: https://github.com/react-navigation/hooks/issues/40

[satya164]

I think this is not the responsibility of react-native-hooks since this changes the semantics of how the navigation object behaves. Refactoring between props.navigation and useNavigation should not change the behaviour or break code. This kind of change should be done in React Navigation core.

Also, we should not hardcode these methods on the navigation object. As you pointed out, methods like openDrawer will now exist on stack navigator which shouldn't be the case. It also means that for navigators that are not handled here will behave differently, for example, tab navigator has a jumpTo method which won't be stable like all other methods here.

A better solution is to send a PR to core to make these methods stable :)

[neiker]

There's an issue already closed on core repo for this: https://github.com/react-navigation/core/issues/71

This should be here because its too dificult to make that change on core right now and also because v5 it's coming and this package will be deprecated.

[neiker]

Doing more research I found there's too many posible bugs with this implementation. More info here: https://github.com/facebook/react/issues/16956

Maybe it's just simpler let this as it is and add a warning so users can just add this helper to theirs code base:

function useStableCallback(cb: (...args: any) => any) {
  const ref = useRef(cb);

  useLayoutEffect(() => {
    ref.current = cb;
  }, [cb]);

  return useCallback((...args) => ref.current(...args), [ref]);
};

[satya164]

its too dificult to make that change on core right now

I don't think it's too difficult. The action creators are created in one place it should be possible to use a similar approach by storing these in a cache object instead ref.

It's more work but it's also correct and handles way more cases than you can handle here without complicating code.

and also because v5 it's coming and this package will be deprecated

That doesn't mean we should make it inconsistent with the version of React Navigation we use it with.

Anyway, as I already mentioned, there are many issues with this approach.

• Hardcoding shouldn't be done because
  • There can be multiple navigators. e.g. currently your code doesn't handle jumpTo for tab. Custom navigators can have their own methods as well.
  • Users can provide their custom action creators. This doesn't handle it.
  • There are other methods in React Navigation as well. Methods like emit are not covered and hardcoding means any changes/additions will make this code incorrect.
• It makes navigation object semantically different. You refactor from useNavigation to props.navigation and now the code breaks. This shouldn't happen.
• Methods like getParam and isFirstRouteInParent should not be stable.

[slorber]

Good points, thanks @satya164

I'm closing the PR then

It makes navigation object semantically different. You refactor from useNavigation to props.navigation and now the code breaks. This shouldn't happen.

Agree on this, and also a reason I don't think we should fix it here but in core instead.

Temporarily, I think the best option would be to recommend users to fix this in userland, with a workaround like

function useNavigationRef() {
  const navigation = useNavigation();
  const ref = useRef(navigation);

  useLayoutEffect(() => {
    ref.current = navigation;
  }, [navigation]);

  return ref;
};

const navigationRef = useNavigationRef();
  useEffect(() => {
      if (isAuthenticated) {
          navigationRef.current.navigate("AuthenticatedNavigator")
      } else {
          navigationRef.current.navigate("UnauthenticatedNavigator")
      }
  }, [isAuthenticated,navigationRef]) 

What do you think?